Bug 202392 – [JSC] Thread JSGlobalObject* instead of ExecState*

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

[Yusuke Suzuki]

But, it is super costly if we ends up calling lexicalGlobalObject().
So, we need to put JSGlobalObject* in JSFunction/InternalFunction. And trampoline for them will get objects and pass them to the host-function.
Then, the host-function signature is changed to `hostFunction(CallFrame*, JSGlobalObject*)`. CallFrame* is necessary since we still need to access thisValue, arguments etc. residing on the stack.

[Yusuke Suzuki]

I think this is the simplest way given the followings

1. InternalFunction is not so many
2. sizeof(JSFunction) is 40. Increasing to 48 is OK.

[Radar WebKit Bug Importer]

<rdar://problem/55881584>

[Yusuke Suzuki]

The patch becomes too large, I'll spawn part of it.

[Yusuke Suzuki]

Created attachment 380709 [details]
Patch

[Yusuke Suzuki]

Created attachment 380719 [details]
Patch

[Yusuke Suzuki]

Created attachment 381157 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Yusuke Suzuki]

Created attachment 381160 [details]
Patch

[Yusuke Suzuki]

Created attachment 381162 [details]
Patch

[Yusuke Suzuki]

(In reply to Build Bot from comment #7)
> Thanks for the patch. If this patch contains new public API please make sure
> it follows the guidelines for new WebKit2 GTK+ API. See
> http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

This is unrelated.

[Yusuke Suzuki]

Created attachment 381163 [details]
Patch

[Yusuke Suzuki]

Created attachment 381166 [details]
Patch

[Yusuke Suzuki]

Created attachment 381172 [details]
Patch

[Yusuke Suzuki]

Created attachment 381173 [details]
Patch

[Yusuke Suzuki]

Created attachment 381179 [details]
Patch

[Yusuke Suzuki]

Created attachment 381229 [details]
Patch

[Yusuke Suzuki]

Created attachment 381246 [details]
Patch

[EWS Watchlist]

Comment on attachment 381246 [details]
Patch

Attachment 381246 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13145189

New failing tests:
fast/events/attribute-listener-cloned-from-frameless-doc-context-2.html
fast/events/attribute-listener-extracted-from-frameless-doc-context-2.html
fast/dom/HTMLObjectElement/object-as-frame.html

[EWS Watchlist]

Created attachment 381259 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Yusuke Suzuki]

Created attachment 381275 [details]
Patch

[Yusuke Suzuki]

Created attachment 381282 [details]
Patch

[Yusuke Suzuki]

Created attachment 381287 [details]
Patch

[Yusuke Suzuki]

Created attachment 381365 [details]
Patch

[EWS Watchlist]

Comment on attachment 381365 [details]
Patch

Attachment 381365 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13150254

New failing tests:
apiTests

[Yusuke Suzuki]

Created attachment 381431 [details]
Patch

[Yusuke Suzuki]

Created attachment 381484 [details]
Patch

[Yusuke Suzuki]

Created attachment 381488 [details]
Patch

[Yusuke Suzuki]

Created attachment 381490 [details]
Patch

[Yusuke Suzuki]

Created attachment 381492 [details]
Patch

[Geoffrey Garen]

Comment on attachment 381492 [details]
Patch

r=me

This patch makes me happy. Good luck landing it :P.

[Yusuke Suzuki]

(In reply to Geoffrey Garen from comment #30)
> Comment on attachment 381492 [details]
> Patch
> 
> r=me
> 
> This patch makes me happy. Good luck landing it :P.

This removes many of ExecState::vm() calls (we now call JSGlobalObject::vm() instead). And it allows us to use LargeAllocation for any JSCells, and then, it allows us to introduce lower tier of IsoSubspace allocation (similar to the thing done in IsoHeap). And... We start applying IsoSubspaces to all the JSObjects!

[Yusuke Suzuki]

The JSC failure is due to my bug I added in the last patch (using COMPILER(GCC_OR_CLANG)). We should use `COMPILER(GCC_COMPATIBLE)`.

[Yusuke Suzuki]

Created attachment 381507 [details]
Patch

[Geoffrey Garen]

Another really nice feature of this patch is that it creates a clearer API to WebCore and WebKit programmers.

[Keith Miller]

rs=me. I have a few comments from the parts that looked interesting: 

Can you explain the m_functionConstructor/m_regExpConstructor changes in JSGlobalObject in the ChangeLog? I'm guessing it was for a bug somewhere that was using the wrong value or something.

Can you add some comment to WebCore's ChangeLog explaining why we are making this change?

I also didn't know about `__builtin_frame_address`, which is pretty cool!

[Yusuke Suzuki]

(In reply to Geoffrey Garen from comment #34)
> Another really nice feature of this patch is that it creates a clearer API
> to WebCore and WebKit programmers.

Yeah, we decouple lexical JSGlobalObject from CallFrame, making CallFrame actually just a CallFrame :)

[Yusuke Suzuki]

(In reply to Keith Miller from comment #35)
> rs=me. I have a few comments from the parts that looked interesting: 
> 
> Can you explain the m_functionConstructor/m_regExpConstructor changes in
> JSGlobalObject in the ChangeLog? I'm guessing it was for a bug somewhere
> that was using the wrong value or something.

It is just used in some places to remove CallFrame* dependencies :) I'll note it.

> 
> Can you add some comment to WebCore's ChangeLog explaining why we are making
> this change?

Yeah :)

> 
> I also didn't know about `__builtin_frame_address`, which is pretty cool!

[Yusuke Suzuki]

Created attachment 381517 [details]
Patch

[Yusuke Suzuki]

Created attachment 381523 [details]
Patch

[Yusuke Suzuki]

Hmm, I'm attempting to make 32bit JIT built, but seems that EWS for these builds is too slow, and I cannot get the result. I'll land this first and check the buildbot status after that, otherwise, this patch needs to be rebaselined forever...

[Yusuke Suzuki]

Committed r251425: <https://trac.webkit.org/changeset/251425>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #40)
> Hmm, I'm attempting to make 32bit JIT built, but seems that EWS for these
> builds is too slow, and I cannot get the result. I'll land this first and
> check the buildbot status after that, otherwise, this patch needs to be
> rebaselined forever...

https://bugs.webkit.org/show_bug.cgi?id=203242

[Saam Barati]

LGTM too

[Yusuke Suzuki]

I'll do follow-up patch to improve things more.

1. Add more API tests using vm.topCallFrame related things from C++ world
2. Add assertions which makes DECLARE_CALL_FRAME safer

[Yusuke Suzuki]

Committed r251459: <https://trac.webkit.org/changeset/251459>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #44)
> I'll do follow-up patch to improve things more.
> 
> 1. Add more API tests using vm.topCallFrame related things from C++ world
> 2. Add assertions which makes DECLARE_CALL_FRAME safer

Do this in https://bugs.webkit.org/show_bug.cgi?id=203274

[Yusuke Suzuki]

Another stabilizing patch https://bugs.webkit.org/show_bug.cgi?id=203285

[Pablo Saavedra]

(In reply to Yusuke Suzuki from comment #40)
> Hmm, I'm attempting to make 32bit JIT built, but seems that EWS for these
> builds is too slow, and I cannot get the result. I'll land this first and
> check the buildbot status after that, otherwise, this patch needs to be
> rebaselined forever...

(bug Reopened)

It seems that the EWS for are experimenting troubles. Unfortunately, it looks like the https://trac.webkit.org/r251425 is breaking the JSC test suite in https://build.webkit.org/waterfall?category=misc right now.

Hopefully you could take a look into this.

[Zan Dobersek]

(In reply to Pablo Saavedra from comment #48)
> (In reply to Yusuke Suzuki from comment #40)
> > Hmm, I'm attempting to make 32bit JIT built, but seems that EWS for these
> > builds is too slow, and I cannot get the result. I'll land this first and
> > check the buildbot status after that, otherwise, this patch needs to be
> > rebaselined forever...
> 
> (bug Reopened)
> 
> It seems that the EWS for are experimenting troubles. Unfortunately, it
> looks like the https://trac.webkit.org/r251425 is breaking the JSC test
> suite in https://build.webkit.org/waterfall?category=misc right now.
> 
> Hopefully you could take a look into this.

The main issue from this patch is that there's a missing call to JIT::prepareCallOperation(VM&) in DFG::OSRExit::emitRestoreArguments() which would properly adjust the vm.topCallFrame value. This trips up OSR exits, resulting in crashes.

Besides that, there are other places in DFG where a call operation doesn't invoke prepareCallOperation(), but without any consequences.

I'd post a patch for it already, but r251468 broke things in a different way, so this is just a heads-up about the solution.

[Yusuke Suzuki]

(In reply to Zan Dobersek from comment #49)
> (In reply to Pablo Saavedra from comment #48)
> > (In reply to Yusuke Suzuki from comment #40)
> > > Hmm, I'm attempting to make 32bit JIT built, but seems that EWS for these
> > > builds is too slow, and I cannot get the result. I'll land this first and
> > > check the buildbot status after that, otherwise, this patch needs to be
> > > rebaselined forever...
> > 
> > (bug Reopened)
> > 
> > It seems that the EWS for are experimenting troubles. Unfortunately, it
> > looks like the https://trac.webkit.org/r251425 is breaking the JSC test
> > suite in https://build.webkit.org/waterfall?category=misc right now.
> > 
> > Hopefully you could take a look into this.
> 
> The main issue from this patch is that there's a missing call to
> JIT::prepareCallOperation(VM&) in DFG::OSRExit::emitRestoreArguments() which
> would properly adjust the vm.topCallFrame value. This trips up OSR exits,
> resulting in crashes.
> 
> Besides that, there are other places in DFG where a call operation doesn't
> invoke prepareCallOperation(), but without any consequences.
> 
> I'd post a patch for it already, but r251468 broke things in a different
> way, so this is just a heads-up about the solution.

Thanks. I'm doing a bit mechanical way to find missing prepareCallOperation(VM&) in https://bugs.webkit.org/show_bug.cgi?id=203285
Can you try the part of this patch in your local 32bit JIT machine and test it?

[Yusuke Suzuki]

32bit issues will be fixed in bug 203285

[Yusuke Suzuki]

*** Bug 134932 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

*** Bug 202340 has been marked as a duplicate of this bug. ***

[EWS]

Committed 262166@main (7a682381645f): <https://commits.webkit.org/262166@main>

Reviewed commits have been landed. Closing PR #11084 and removing active labels.