JSStdFunction is now implemented in JSFunction.cpp. And it has the memory leak; It does not have destroy method (and it's not destructible), but it has the std::function as the member. Since std::function accepts the lambda, it allocates the memory from the heap. So we need to call the destructor onto this.
When I tried to use it to implement Asynchronous error reporting for the module loaders, I've found the issue. So I created the separate patch. https://bugs.webkit.org/show_bug.cgi?id=148173
Seems weird to move the std::function to the FunctionRareData. We'll have to be careful, since we don't want to bloat FunctionRareData too much.
(In reply to comment #2) > Seems weird to move the std::function to the FunctionRareData. We'll have > to be careful, since we don't want to bloat FunctionRareData too much. Yeah, you're right. After starting the prototyping, I found that FunctionRareData is tightly coupled with the object allocation profile. Adding the non-related-to-profile data to it seems not good. I'm now investigating the best way to do.
(In reply to comment #3) > (In reply to comment #2) > > Seems weird to move the std::function to the FunctionRareData. We'll have > > to be careful, since we don't want to bloat FunctionRareData too much. > > Yeah, you're right. > After starting the prototyping, I found that FunctionRareData is tightly > coupled with the object allocation profile. Adding the > non-related-to-profile data to it seems not good. I'm now investigating the > best way to do. Having a NativeStdFunctionCell that works like FunctionRareData but only for JSStdFunction was my other proposal. I suggested reusing FunctionRareData because I wasn't sure how much we wanted to keep it coupled with the object allocation profile.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Seems weird to move the std::function to the FunctionRareData. We'll have > > > to be careful, since we don't want to bloat FunctionRareData too much. > > > > Yeah, you're right. > > After starting the prototyping, I found that FunctionRareData is tightly > > coupled with the object allocation profile. Adding the > > non-related-to-profile data to it seems not good. I'm now investigating the > > best way to do. > > Having a NativeStdFunctionCell that works like FunctionRareData but only for > JSStdFunction was my other proposal. I suggested reusing FunctionRareData > because I wasn't sure how much we wanted to keep it coupled with the object > allocation profile. It seems nice. Or creating the new Executable class (like NativeLambdaExecutable or something) that inherits NativeExecutable and holds std::function is one idea.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > Seems weird to move the std::function to the FunctionRareData. We'll have > > > > to be careful, since we don't want to bloat FunctionRareData too much. > > > > > > Yeah, you're right. > > > After starting the prototyping, I found that FunctionRareData is tightly > > > coupled with the object allocation profile. Adding the > > > non-related-to-profile data to it seems not good. I'm now investigating the > > > best way to do. > > > > Having a NativeStdFunctionCell that works like FunctionRareData but only for > > JSStdFunction was my other proposal. I suggested reusing FunctionRareData > > because I wasn't sure how much we wanted to keep it coupled with the object > > allocation profile. > > It seems nice. Or creating the new Executable class (like > NativeLambdaExecutable or something) that inherits NativeExecutable and > holds std::function is one idea. This is probably better, actually.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > (In reply to comment #2) > > > > > Seems weird to move the std::function to the FunctionRareData. We'll have > > > > > to be careful, since we don't want to bloat FunctionRareData too much. > > > > > > > > Yeah, you're right. > > > > After starting the prototyping, I found that FunctionRareData is tightly > > > > coupled with the object allocation profile. Adding the > > > > non-related-to-profile data to it seems not good. I'm now investigating the > > > > best way to do. > > > > > > Having a NativeStdFunctionCell that works like FunctionRareData but only for > > > JSStdFunction was my other proposal. I suggested reusing FunctionRareData > > > because I wasn't sure how much we wanted to keep it coupled with the object > > > allocation profile. > > > > It seems nice. Or creating the new Executable class (like > > NativeLambdaExecutable or something) that inherits NativeExecutable and > > holds std::function is one idea. > > This is probably better, actually. Putting this in executable makes the most sense, IMO.
(In reply to comment #7) > Putting this in executable makes the most sense, IMO. After investigating the code, I think holding NativeStdFunctionCell* in JSFunction is better than introducing NativeStdExecutable inheriting ExecutableBase (or NativeExecutable). Here is the reason. 1. Each NativeExecutable (in 64_32 JIT environment) has the trampoline to call the given host function. And this trampoline has the function address as the immediate value within the JIT-compiled trampoline code. 2. To suppress the overuse of the executable memory (which is used to generate the trampoline), NativeExecutable is cached. The host function address is used as the key to look up the cached executable from the table. 3. In all the JSStdFunction, we use the same host function that immediately calls the each std::function. 4. As a result, without any change, all the JSStdFunction hit the cached NativeExecutable even if the held std::function is different. 5. To solve it, if we put the std::function in the NativeExecutable, we need to add this std::function identity (like address) to the cache key to distinguish the NativeExecutables generated from the different JSStdFunctions, because the address of the stub host function (that calls the std::function) is the same in the all JSStdFunction. 6. But since the std::function will be allocated in the heap, this address is always different. So caching has no effect. 7. If we do not cache the NativeExecutable that holds the std::function, each time when creating the JSStdFunction, we need to regenerate the completely same trampolines (since it just calls the same host function stub that calls the std::function). So I think separating the NativeExecutable from the std::function is preferable. By doing so, all the JSStdFunction can use the same NativeExecutable.
Created attachment 260122 [details] Patch
Comment on attachment 260122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260122&action=review > Source/JavaScriptCore/runtime/JSArrowFunction.h:-52 > - JSArrowFunction does not have any fields that need to be destructed. Just drop the `destroy` method. Note: Since JSFunction is not destructible object, this `destroy` is not called anyway.
Created attachment 260124 [details] Patch
Created attachment 260167 [details] Patch
Comment on attachment 260167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260167&action=review > Source/JavaScriptCore/runtime/JSFunction.cpp:107 > static EncodedJSValue JSC_HOST_CALL runStdFunction(ExecState* state) > { > - JSStdFunction* jsFunction = jsCast<JSStdFunction*>(state->callee()); > + JSNativeStdFunction* jsFunction = jsCast<JSNativeStdFunction*>(state->callee()); > ASSERT(jsFunction); > - return jsFunction->stdFunction(state); > + return jsFunction->nativeStdFunctionCell()->function()(state); > } > > JSFunction* JSFunction::create(VM& vm, JSGlobalObject* globalObject, int length, const String& name, NativeStdFunction&& nativeStdFunction, Intrinsic intrinsic, NativeFunction nativeConstructor) > { > NativeExecutable* executable = getNativeExecutable(vm, runStdFunction, intrinsic, nativeConstructor); > - JSStdFunction* function = new (NotNull, allocateCell<JSStdFunction>(vm.heap)) JSStdFunction(vm, globalObject, globalObject->functionStructure(), WTF::move(nativeStdFunction)); > - // Can't do this during initialization because getHostFunction might do a GC allocation. > - function->finishCreation(vm, executable, length, name); > + NativeStdFunctionCell* functionCell = NativeStdFunctionCell::create(vm, WTF::move(nativeStdFunction)); > + JSNativeStdFunction* function = new (NotNull, allocateCell<JSNativeStdFunction>(vm.heap)) JSNativeStdFunction(vm, globalObject, globalObject->nativeStdFunctionStructure()); > + function->finishCreation(vm, executable, length, name, functionCell); > return function; It's super weird that this code is in JSFunction and not in JSNativeStdFunction.
Comment on attachment 260167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260167&action=review >> Source/JavaScriptCore/runtime/JSFunction.cpp:107 >> return function; > > It's super weird that this code is in JSFunction and not in JSNativeStdFunction. OK, I'll move it out from the JSFunction.
Created attachment 260176 [details] Patch
Comment on attachment 260176 [details] Patch Great!
Comment on attachment 260176 [details] Patch Clearing flags on attachment: 260176 Committed r189124: <http://trac.webkit.org/changeset/189124>
All reviewed patches have been landed. Closing bug.