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

Require just used functions & use natives #54

Merged
merged 2 commits into from
Jul 27, 2015
Merged

Require just used functions & use natives #54

merged 2 commits into from
Jul 27, 2015

Conversation

luanmuniz
Copy link
Contributor

Hi,

I made this because after a few requests using this module, lodash was causing some memory allocation problems, the full file is very big and it was causing me problems.

I don't quite understand why, but make just these changes, requiring just the used functions and using a few native functions, my application decrease the memory allocation from 500mb to 220mb.

@analog-nico
Copy link
Member

Thanks @luanmuniz !

That is an important finding I would like to understand better so that future versions don't reintroduce this issue. Can you say something about the following questions?

  1. The code pieces you replaced with native functions are only executed once when you require the module. I expect if you revert them to using lodash functions you still won't have the memory issue. Could you try this out?
  2. Apart from the native functions you did (1) require individual lodash functions and (2) update lodash from 3.6.x to ^3.6.0. First of all, which lodash version do you use? Please have a look at /node_modules/request-promise/node_modules/lodash/package.json. This may answer whether (1) or (2) resolved the issue.

@luanmuniz
Copy link
Contributor Author

  1. Yes. When the native is replaced for the lodash version the memory issues its not there. The problem is in calling the entire module for just a few functions. (Full lodash has almost 500kb unminified and the code at npm its not minified)
  2. The 1 is the one that solve the issue. request-promise is using 3.6 right now. I meant to update to 3.10 just to update, not to solve the problem (My mistake pushing just a different call for the same version)
    • Update to 3.10 is a good thing because there are a lot of modules using lodash and most of then use the last version and when you have 2 or more versions at the same application (different dependences using different versions) node.js need to cache all the versions instead of just one.

Do you want me to update the PR in any way?

@analog-nico
Copy link
Member

Thanks for the info @luanmuniz ! I can take it from here. You don't need to update the PR.

analog-nico added a commit that referenced this pull request Jul 27, 2015
Require just used functions & use natives
@analog-nico analog-nico merged commit 225f027 into request:master Jul 27, 2015
@analog-nico
Copy link
Member

I just published version 0.4.3 on NPM which contains your changes. Thanks for your contribution!

@luanmuniz
Copy link
Contributor Author

Thank you very much! :D

@luanmuniz
Copy link
Contributor Author

Just by curiosity, is there a reason not to use the native Object.keys, Array.forEach and the === undefined?

@analog-nico
Copy link
Member

I don't know. I leave the expertise on subtle differences between platforms to lodash. ;)

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

Successfully merging this pull request may close these issues.

2 participants