-
-
Notifications
You must be signed in to change notification settings - Fork 949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Blazing Fast Module Loading 🚀😎 #1921
Conversation
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 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 Going to be a 11.1.0 release anyway. |
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. |
After merging #1898, the booting time increased significantly:
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. |
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. |
b73a3cf
to
d7f1701
Compare
I've updated the branch. Everything should be good now, and the cache has been deleted. |
thanks, the boot time has increased, I think there must be something lingering that shouldn't be |
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. |
they appear to be in the branch still
|
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. |
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!
If you have permission, that would have been the best approach, or you could have asked me or @dcblogdev to do that.
The purpose and changes in this PR are completely different from #1898
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. |
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. |
@alissn no, I've merged it now. |
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 thescan
method in a static variable.Benchmarking
To test this improvement, I created 166 modules and benchmarked the
isEnabled()
method of the package.Benchmark Code:
Results:
Before:
![image](https://private-user-images.githubusercontent.com/26966142/358225274-1f67b314-e56a-4d94-b374-d2afed558779.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMTI0OTgsIm5iZiI6MTcyNjExMjE5OCwicGF0aCI6Ii8yNjk2NjE0Mi8zNTgyMjUyNzQtMWY2N2IzMTQtZTU2YS00ZDk0LWIzNzQtZDJhZmVkNTU4Nzc5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDAzMzYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTdiYmZhNWE0NGVjODAxODI4MGIyYzA5NDgxOTk3NWY5NWZjYzUwMzNhMzg3ZGU2ODliYWEzNWIxNjY0MzllMGQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.vFy-xwIXBwc3zHqFY22ZVwNP-il9O_tLNLREBOFKTfw)
After:
![image](https://private-user-images.githubusercontent.com/26966142/358225447-45beb5d9-34dd-45ce-9649-500afd09cc25.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMTI0OTgsIm5iZiI6MTcyNjExMjE5OCwicGF0aCI6Ii8yNjk2NjE0Mi8zNTgyMjU0NDctNDViZWI1ZDktMzRkZC00NWNlLTk2NDktNTAwYWZkMDljYzI1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDAzMzYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTdlOTIxMjM3MmQyMGY4NTc1MTI1YTJjMDEzOTFhZGEzZTQyNWZlMDY1NmRjNTM0NWFjYzliZmIxZDAwZTcxYWMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.aBMGgCUrIEI_ECaYbYHkXS9d0mhxe738n0wXkvm61Dk)
also check with laravel-debugbar:
![telegram-cloud-photo-size-4-5904411011077751978-y](https://private-user-images.githubusercontent.com/26966142/358288339-3d55ca54-b235-464c-9042-58dd9828bf59.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMTI0OTgsIm5iZiI6MTcyNjExMjE5OCwicGF0aCI6Ii8yNjk2NjE0Mi8zNTgyODgzMzktM2Q1NWNhNTQtYjIzNS00NjRjLTkwNDItNThkZDk4MjhiZjU5LmpwZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDAzMzYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQyNjk3YzFlMzBlODRkZTMzNDViZWMzOTc3ZTYzY2NkMjhjMTEyMzVjNDE4ODJjYmJmYTBlODMwOTgzNWRhZWMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.aSE2VVeMmAzDcsv1paPA1m9onawrHzQYcB_YyNRndDw)
![telegram-cloud-photo-size-4-5904411011077751977-y](https://private-user-images.githubusercontent.com/26966142/358288380-170c7243-608d-43b7-9d87-3be705eda090.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxMTI0OTgsIm5iZiI6MTcyNjExMjE5OCwicGF0aCI6Ii8yNjk2NjE0Mi8zNTgyODgzODAtMTcwYzcyNDMtNjA4ZC00M2I3LTlkODctM2JlNzA1ZWRhMDkwLmpwZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDAzMzYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTI3MWM2ZDAyMDk0OTcyZDU4YTIxMDAzNmU3ODFiZTA4N2MxZjUzNGU3NTljNDgzNzM2N2FiMGY2MDAwMTMyNTcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.izCKuzCKdq9SUCPgR-59CAMBcnPxsebbR-Oiq8UVILA)
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):
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):
This change further enhances performance and efficiency. and can solve #1745
Please review the changes and let me know your thoughts!