Bug 148262 – Move std::function from JSFunction into NativeStdFunctionCell to correctly destroy the heap allocated std::function

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Filip Pizlo]

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.

[Yusuke Suzuki]

(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.

[Basile Clement]

(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.

[Yusuke Suzuki]

(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.

[Basile Clement]

(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.

[Filip Pizlo]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 260122 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260124 [details]
Patch

[Yusuke Suzuki]

Created attachment 260167 [details]
Patch

[Filip Pizlo]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260176 [details]
Patch

[Filip Pizlo]

Comment on attachment 260176 [details]
Patch

Great!

[WebKit Commit Bot]

Comment on attachment 260176 [details]
Patch

Clearing flags on attachment: 260176

Committed r189124: <http://trac.webkit.org/changeset/189124>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.