Skip to content
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

[proposal] Enable module cache during sails load #7161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zohareshed
Copy link

@zohareshed zohareshed commented Aug 5, 2021

The fact that sails doesn't pass the force argument to includeAll causes the several services to being required multiple times which impacts the perfomance.
Lifting time was 2 times quicker when I used the "force" attribute, in addition my app consumed 30% less RAM.

@sailsbot
Copy link

sailsbot commented Aug 5, 2021

Thanks for submitting this pull request, @zohareshed! We'll look at it ASAP.

In the mean time, here are some ways you can help speed things along:

  • discuss this pull request with other contributors and get their feedback. (Reactions and comments can help us make better decisions, anticipate compatibility problems, and prevent bugs.)
  • ask another JavaScript developer to review the files changed in this pull request. (Peer reviews definitely don't guarantee perfection, but they help catch mistakes and enourage collaborative thinking. Code reviews are so useful that some open source projects require a minimum number of reviews before even considering a merge!)
  • if appropriate, ask your business to sponsor your pull request. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
  • make sure you've answered the "why?" (Before we can review and merge a pull request, we feel it is important to fully understand the use case: the human reason these changes are important for you, your team, or your organization.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@Rovack
Copy link

Rovack commented Aug 29, 2021

This would be super helpful! So weird seeing Sails loading the same modules like 10 times during lifting.

I see this has already been waiting for almost a month, and was even brought up on the Gitter with no response.

What would it take for this to be addressed?

@DominusKelvin
Copy link
Contributor

@eashaw have you had any chance to look at this?

@Rovack
Copy link

Rovack commented Sep 22, 2021

I guess that's a "no". 😓

@DominusKelvin @eashaw Is there a standard process for getting something like this reviewed, beyond opening the PR here and posting on Gitter?

@rKozokinG
Copy link

Awesome! This give my team amazing cost reduction and solve a lot of problems we have while using sails!
Would be great if you merge as soon as possible 😄

@ghost
Copy link

ghost commented Oct 4, 2021

Wow this shortend sails lift time in a half! Can you sails team please approve it?

@OneStromberg
Copy link

This is something my team is waiting for a long time, we managed to fix it locally using local npm module, but you know how hard to maintain it! Please, make it as a part of the repo! 🙏

@zohareshed
Copy link
Author

@eashaw @DominusKelvin
Please let me know what I can do in order to push this proposal forward.

@zohareshed zohareshed closed this Oct 26, 2021
@zohareshed zohareshed reopened this Oct 26, 2021
@sailsbot
Copy link

Oh hey again, @zohareshed. Now that this pull request is reopened, it's on our radar. Please let us know if there's any new information we should be aware of!


Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@zohareshed
Copy link
Author

@DominusKelvin @eashaw
Guys this is waiting forever. Can you please look at it already?

@eashaw
Copy link
Member

eashaw commented Oct 29, 2021

@zohareshed, Could you tell us about the issue you're facing with ram?

@zohareshed
Copy link
Author

@eashaw When sails lift, it requires all the Services multiple times. That uses A LOT of ram, and CPU.
This is a PR for optimizing the sails lift performance.

@zohareshed
Copy link
Author

@DominusKelvin @eashaw ???

@eashaw
Copy link
Member

eashaw commented Nov 8, 2021

Hey @zohareshed can you share with us ram/cpu usage with this PR against latest version of Sails (in the case of your app)?

@DominusKelvin
Copy link
Contributor

@zohareshed apologies I just got to checking the progress on this PR. Can you please provide what @eashaw asked?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants