Blazing Fast Module Loading 🚀😎

[alissn]

Hi,

In this pull request, I've updated the logic of the FileRepository to significantly speed up the load time of modules by storing the result of the scan method in a static variable.

Benchmarking

To test this improvement, I created 166 modules and benchmarked the isEnabled() method of the package.

Benchmark Code:

    $modules = [
        "A1"        => "A1",
        "A2"        => "A2",
        "Ali"       => "Ali",
        "Auth"      => "Auth",
        "Module0"   => "Module0",
        "Module1"   => "Module1",
        "Module10"  => "Module10",
        // other modules name
        "Module98"  => "Module98",
        "Module99"  => "Module99",
        "Profile"   => "Profile",
    ];

    \Illuminate\Support\Benchmark::dd([
        'check1' => function () use ($modules) {
            $module = array_rand($modules);
            $a      = \Nwidart\Modules\Facades\Module::isEnabled($module);
        },
    ], 30);

Results:

Before:

After:

also check with laravel-debugbar:
Before:

After:

As you can see, the results are impressive! 🚀🚀🚀😎

Additional Improvements:

I've also optimized the logic for finding modules. Previously, the module search was done inside a loop with a complexity of O(n):

 foreach ($this->all() as $module) {
    if ($module->getLowerName() === strtolower($name)) {
        return $module;
    }
}

Now, I've changed the storage keys to lowercase when storing, and I check for module existence with an array key, reducing the complexity to O(1):

   return $this->all()[strtolower($name)] ?? null;

This change further enhances performance and efficiency. and can solve #1745

Please review the changes and let me know your thoughts!

[dcblogdev]

this is a great improvement!! I have tested this with 154 modules. Tested this on https://github.com/laravel-modules-com/breeze-demo

Typical load times:

1 x Booting (98.3%) | 1.16s
1 x Application (1.7%) | 19.84ms

Caching actually makes the system slower, so maybe an idea to remove the caching?

With caching enabled the booting time increases

1 x Booting (99.63%) | 5.77s
1 x Application (0.37%) | 21.26ms

Going to be a 11.1.0 release anyway.

[alissn]

Hi, I agree with you about deleting the cache.

Using a static method to store module data is indeed very fast and keeps the data in memory.

I'll remove the cache logic in a few minutes.

[alissn]

@dcblogdev

After merging #1898, the booting time increased significantly:

• Booting: 99.76% | 5.65s
• Application: 0.24% | 13.69ms

It appears that many methods were changed, and I can't pinpoint exactly what was altered.

I initially chose not to review #1898 because it included various changes and seemed unnecessary to rename methods. Now, we have to update all the documentation, which will take a lot of time.

I suggest we revert the merge #1898 and create a new one with smaller, more manageable changes that can be reviewed more easily.

[dcblogdev]

Yes I agree I have a copy of your changes to this file so I can do a replace of the file contents unless you want to do it, just taking the dogs out. Will be back later.

[alissn]

@dcblogdev

please revert merge #1898, I can push the updated caching changes to this branch. After merging this PR, some of the changes from #1898 might not be necessary.

Alternatively, we could review #1898 and split it into smaller, more manageable merge requests.

[alissn]

@dcblogdev

I've updated the branch. Everything should be good now, and the cache has been deleted.

[dcblogdev]

thanks, the boot time has increased, I think there must be something lingering that shouldn't be

[alissn]

@dcblogdev

Hi, I noticed you also reverted the changes in this commit.

This might have increased the boot time due to that incident. This pull request is related to #1920.

[dcblogdev]

they appear to be in the branch still

laravel-modules/src/LaravelModulesServiceProvider.php

Line 122 in 6d57d44

protected function registerTransactions(): void

[solomon-ochepa]

@dcblogdev

After merging #1898, the booting time increased significantly:

• Booting: 99.76% | 5.65s
• Application: 0.24% | 13.69ms

It appears that many methods were changed, and I can't pinpoint exactly what was altered.

I initially chose not to review #1898 because it included various changes and seemed unnecessary to rename methods. Now, we have to update all the documentation, which will take a lot of time.

I suggest we revert the merge #1898 and create a new one with smaller, more manageable changes that can be reviewed more easily.

No logic was modified, and all the changes are well documented in the PR. All you have to do is merge/rebase the main to your branch and it will apply all the changes automatically. Those that can't be merged automatically will be listed under merge changes and you can easily fix them.

I also noticed after reverting the PR, the issue persisted as pointed out by @dcblogdev.

Always do proper testing before concluding.

[solomon-ochepa]

Please revert merge #1898

When a PR is merged, don't ask for it to be reverted simply because it doesn't tally with your work-in-progress branch. You could rebase/merge the main branch, and fix any changes.

If there are any breaking changes you are expected to update your PR.

The code will not remain the same forever, and changes are inevitable!

I can push the updated caching changes to this branch.

If you have permission, that would have been the best approach, or you could have asked me or @dcblogdev to do that.

... After merging this PR, some of the changes from #1898 might not be necessary.

The purpose and changes in this PR are completely different from #1898

Alternatively, we could review #1898 and split it into smaller, more manageable merge requests.

The changes are best reviewed as a whole since they involve the normalization (formatting, rearrangement, and renaming) of methods and properties.

Thank you, and I really appreciate all your time and contributions.

I'll be resubmitting the PR again after yours is merged.

[alissn]

Hi @dcblogdev,

Is there any issue preventing the merging and release of the new version of the package?

In my opinion, the last two comments aren't critical and don't require a response.

[dcblogdev]

@alissn no, I've merged it now.