Bug 205327 – [JSC] Improve our bound function implementation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

I think there are bunch of things to do.

1. We should not look up JIT thunks every time we create JSBoundFunction.
2. We should have fast path for bound args. (I think it is easy).

And more.

[Yusuke Suzuki]

Created attachment 386181 [details]
Patch

[Yusuke Suzuki]

Created attachment 386183 [details]
Patch

[Yusuke Suzuki]

Created attachment 386234 [details]
Patch

[Yusuke Suzuki]

Created attachment 386238 [details]
Patch

[Keith Miller]

Comment on attachment 386238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386238&action=review

r=me.

> Source/JavaScriptCore/ChangeLog:10
> +        1. Rename CallFrameSlot::argumentCount to CallFrameSlot::argumentCountIncludingThis.

Nooo, the merge conflicts! :P

> Source/JavaScriptCore/ChangeLog:15
> +        3. Cache NativeExecutable for JSBoundFunction in VM. Using hash-map in JITThunk for NativeExecutable is assuming that host-function creation is not done by
> +           user program: each function is pre-defined one by the environment, and there is no case creating host-functions repeatedly from user-program. This is not
> +           true for JSBoundFunction. We should cache NativeExecutable in VM to look up fast.

If I'm understanding what you are trying to say correctly I think I would phrase this as:

Cache NativeExecutable for JSBoundFunction in the VM. We use a hash-map in JITThunk for NativeExecutables because we assume that host-function creation cannot not done by
the user program: each executable is pre-defined to exactly one object by the environment, and there is no way to create host-functions repeatedly from the user-program. The only exception to this is
JSBoundFunction so caching it on the VM avoids the hash-map lookup.
true for JSBoundFunction.

> Source/JavaScriptCore/ChangeLog:17
> +           bound arguments. And it is used as event listener. This is really bad: when dispatching an event, we first call this function from C++, entering JS world,

nit: And it is used => Additionally, it is used. And isn't normally used to start a sentence.

> Source/JavaScriptCore/ChangeLog:19
> +           calling bound JS Executable from thunk. Previously, bound arguments are stored in JSArray. But it is difficult to access them from thunk since we need to consider

nit: calling the* bound JSExecutable from the* thunk.

[Yusuke Suzuki]

Comment on attachment 386238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386238&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:10
>> +        1. Rename CallFrameSlot::argumentCount to CallFrameSlot::argumentCountIncludingThis.
> 
> Nooo, the merge conflicts! :P

Lol, oops, sorry :P

>> Source/JavaScriptCore/ChangeLog:15
>> +           true for JSBoundFunction. We should cache NativeExecutable in VM to look up fast.
> 
> If I'm understanding what you are trying to say correctly I think I would phrase this as:
> 
> Cache NativeExecutable for JSBoundFunction in the VM. We use a hash-map in JITThunk for NativeExecutables because we assume that host-function creation cannot not done by
> the user program: each executable is pre-defined to exactly one object by the environment, and there is no way to create host-functions repeatedly from the user-program. The only exception to this is
> JSBoundFunction so caching it on the VM avoids the hash-map lookup.
> true for JSBoundFunction.

Right. Fixed.

>> Source/JavaScriptCore/ChangeLog:17
>> +           bound arguments. And it is used as event listener. This is really bad: when dispatching an event, we first call this function from C++, entering JS world,
> 
> nit: And it is used => Additionally, it is used. And isn't normally used to start a sentence.

Fixed.

>> Source/JavaScriptCore/ChangeLog:19
>> +           calling bound JS Executable from thunk. Previously, bound arguments are stored in JSArray. But it is difficult to access them from thunk since we need to consider
> 
> nit: calling the* bound JSExecutable from the* thunk.

Fixed.

[Yusuke Suzuki]

Committed r253867: <https://trac.webkit.org/changeset/253867>

[Radar WebKit Bug Importer]

<rdar://problem/58139617>

[Yusuke Suzuki]

Committed r253869: <https://trac.webkit.org/changeset/253869>

[Yusuke Suzuki]

Committed r253870: <https://trac.webkit.org/changeset/253870>