Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

refactor NodeRequest to stop leaking #5529

Merged
merged 4 commits into from
Jul 14, 2016
Merged

refactor NodeRequest to stop leaking #5529

merged 4 commits into from
Jul 14, 2016

Conversation

mikemorris
Copy link
Contributor

Stupid fancy refactored code still fails leak test. Will investigate further tomorrow.

/cc @tmpsantos

mbgl::FileSource::Callback fileSourceCallback_)
: AsyncWorker(new Nan::Callback([&] {
Nan::EscapableHandleScope scope;
return scope.Escape(Nan::New<v8::Function>(
Copy link
Contributor

@tmpsantos tmpsantos Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that v8::Function has its lifetime bound to the context.

@mikemorris mikemorris force-pushed the node-request branch 2 times, most recently from 0089a44 to 76427ec Compare July 13, 2016 19:42
@mikemorris mikemorris added the Node.js node-mapbox-gl-native label Jul 14, 2016
@mikemorris
Copy link
Contributor Author

mikemorris commented Jul 14, 2016

This pull request now implements an alternative to #5527 that solves the v8::FunctionTemplate memory leak while preserving the callback-passing function factory API, but at the cost of creating a new v8::Context for each callback it returns to JavaScript-land.

This is roughly equivalent to creating an iframe to run each function in, and seems to be necessary because of the architecture of v8::FunctionTemplate and v8::Function, which are designed to only be called once in a single context (the global context by default), and calling the function simply creates a new instance of it.

The problem here is that attaching data (like a reference to the NodeRequest object) must be done early, during the creation of the v8::FunctionTemplate, and an instance of the function cannot be created in C++ then passed on to JavaScript (to attach data later with Nan::SetInternalFieldPointer or v8::SetHiddenValue) as calling v8::Function::NewInstance actually executes the function.

In terms of CPU time and memory, it might seem an expensive operation to create a new execution context given the number of built-in objects that must be built. However, V8's extensive caching ensures that, while the first context you create is somewhat expensive, subsequent contexts are much cheaper.

According to the V8 Embedder's Guide, this shouldn't be TOO costly, and has been benchmarked to verify that performance is acceptable.

Changing the API to req.respond, as in #5527, could still possibly be the more performant option, albeit less idiomatic JavaScript, and may be worth considering in the future.

/cc @miccolis @bsudekum @tmpsantos @brunoabinader @jfirebaugh

tmpsantos and others added 4 commits July 14, 2016 13:47
drop NodeRequest::Create, move MakeCallback to NodeRequest::Execute

rework Respond -> HandleCallback

modern NAN style updates
@jfirebaugh
Copy link
Contributor

This looks good to me.

I wonder what the reason is for the v8 design decision to make FunctionTemplates non-garbage collectable... pretty crazy that you have to make an entire context just to get a temporary function.

@mikemorris mikemorris merged commit 7df0e45 into master Jul 14, 2016
@mikemorris mikemorris deleted the node-request branch July 14, 2016 20:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Node.js node-mapbox-gl-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants