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

Defer using process.nextTick #17

Merged
merged 1 commit into from Nov 29, 2016
Merged

Defer using process.nextTick #17

merged 1 commit into from Nov 29, 2016

Conversation

@mcollina
Copy link
Contributor

mcollina commented Nov 29, 2016

Fixes hapijs/hapi#3347.

I have used top-level functions to avoid allocating closures in a hot path.

@arb

This comment has been minimized.

Copy link

arb commented Nov 29, 2016

Curious if you've tested this change or if this is just an optimization on a hot path that might help?

@mcollina

This comment has been minimized.

Copy link
Contributor Author

mcollina commented Nov 29, 2016

@arb I've tested it resolves hapijs/hapi#3347, before this change I could easily get Hapi to 1GB of RAM usage, with this it stays put at 100-150MB.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 29, 2016

Out of curiosity, why the wraps?

@mcollina

This comment has been minimized.

Copy link
Contributor Author

mcollina commented Nov 29, 2016

@AdriVanHoudt because there is no need to allocate a new closure, which it will hold a reference to all the above variables. In this way, most of those can be easily collected.

@hueniverse hueniverse added this to the 1.2.5 milestone Nov 29, 2016
@hueniverse hueniverse self-assigned this Nov 29, 2016
@hueniverse hueniverse merged commit fc4825d into hapijs:master Nov 29, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hueniverse added a commit that referenced this pull request Nov 29, 2016
@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Nov 29, 2016

Stylistically, we don't mind those functions not being placed on internals?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 29, 2016

We do. See the cleanup commit.

@lostthetrail

This comment has been minimized.

Copy link

lostthetrail commented Nov 29, 2016

@mcollina Also confirmed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.