Bug 194648 – [JSC] LLIntEntryPoint creates same DirectJITCode for all functions

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

[Caio Lima]

These objects are essentially the same and seems to be unchanged during execution. We should use singleton to avoid unnecessary allocations. The same applies too all entry points (Function, Eval, Program and Modules).

[Caio Lima]

Created attachment 362008 [details]
WIP - Patch

This is just the initial idea. I want to see how bots handle it.

[EWS Watchlist]

Comment on attachment 362008 [details]
WIP - Patch

Attachment 362008 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11145631

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 362009 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362008 [details]
WIP - Patch

Attachment 362008 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11145691

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 362010 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Caio Lima]

Created attachment 362036 [details]
WIP - Patch

Testing things on EWS

[Mark Lam]

Comment on attachment 362036 [details]
WIP - Patch

This is wrong.  The CTIStub thunks are unique per VM.  You cannot make it a singleton because it is not shared across VMs.  However, I think you can make is one instance per VM.

[Caio Lima]

(In reply to Mark Lam from comment #7)
> Comment on attachment 362036 [details]
> WIP - Patch
> 
> This is wrong.  The CTIStub thunks are unique per VM.  You cannot make it a
> singleton because it is not shared across VMs.  However, I think you can
> make is one instance per VM.

Oh good to know. I'll change this.

[Mark Lam]

(In reply to Caio Lima from comment #8)
> (In reply to Mark Lam from comment #7)
> > Comment on attachment 362036 [details]
> > WIP - Patch
> > 
> > This is wrong.  The CTIStub thunks are unique per VM.  You cannot make it a
> > singleton because it is not shared across VMs.  However, I think you can
> > make is one instance per VM.
> 
> Oh good to know. I'll change this.

On the other hand, I wonder if there's an opportunity here to turn some of these thinks into singletons since they are only trampolines to jump to C++ code.  That would need more careful investigations though.

[Caio Lima]

Created attachment 362129 [details]
WIP - Patch

I want to see EWS. Also I have some questions:

1. `CodeBlock::setJITCode` calls `heap()->reportExtraMemoryAllocated(code->size())`. I'm not sure if this is correct now that we are allocating only one DirectJITCode per VM on LLIntEntryCode;

2. I'm using a lock to avoid concurrency on allocation, but IIUC, current clients of `setFunctionEntrypoint` is LLIntSlowpaths and I'm not sure if they can be executed concurrently. In this case, it's better be conservative.

[Caio Lima]

(In reply to Mark Lam from comment #9)
> (In reply to Caio Lima from comment #8)
> > (In reply to Mark Lam from comment #7)
> > > Comment on attachment 362036 [details]
> > > WIP - Patch
> > > 
> > > This is wrong.  The CTIStub thunks are unique per VM.  You cannot make it a
> > > singleton because it is not shared across VMs.  However, I think you can
> > > make is one instance per VM.
> > 
> > Oh good to know. I'll change this.
> 
> On the other hand, I wonder if there's an opportunity here to turn some of
> these thinks into singletons since they are only trampolines to jump to C++
> code.  That would need more careful investigations though.

I'll investigate this. (https://bugs.webkit.org/show_bug.cgi?id=194715)

[Caio Lima]

It is counter-intuitive, but according to my measuring using massif, the version with this patch is actually taking more memory than baseline version...

[EWS Watchlist]

Comment on attachment 362129 [details]
WIP - Patch

Attachment 362129 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11162282

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362148 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Caio Lima]

Created attachment 362306 [details]
WIP - Patch

[Caio Lima]

Created attachment 362313 [details]
WIP - Patch

Lets see EWS.

[EWS Watchlist]

Attachment 362313 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntThunks.cpp:92:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 362326 [details]
Patch

Properly using LazyNeverDestroyed.
IIUC, It is possible to have a JITed thunk that is shared among all VMs, mainly in the case of LLIntEntrypoints where the code is always the same.
@Mark Lam, do you mind take a look to see if I'm not doing anything wrong here?

[Caio Lima]

Even with EWS green, I have some questions about the proposed Patch:

1. `generateThunkWithJumpTo` generates the thunk to jump to enable other JIted code execute. However, I'm having troubles to understand what guarantees this allocated memory will never be destroyed.

2. Let's suppose we have 2 VM's running concurrently (v1 and v2) and v1 executes `functionForCallEntryThunk` for the first time. The thunk will be allocated and installed under `v1` execution and then `v2` also calls `functionForCallEntryThunk` afterwards. It will not execute `generateThunkWithJumpTo` because it is already generated and then we return the same value we returned to `v1` before. Is the code installed during `v1` execution visible to `v2`?

[Caio Lima]

Created attachment 362897 [details]
Patch

[Caio Lima]

(In reply to Caio Lima from comment #19)
> Even with EWS green, I have some questions about the proposed Patch:
> 
> 1. `generateThunkWithJumpTo` generates the thunk to jump to enable other
> JIted code execute. However, I'm having troubles to understand what
> guarantees this allocated memory will never be destroyed.
> 
> 2. Let's suppose we have 2 VM's running concurrently (v1 and v2) and v1
> executes `functionForCallEntryThunk` for the first time. The thunk will be
> allocated and installed under `v1` execution and then `v2` also calls
> `functionForCallEntryThunk` afterwards. It will not execute
> `generateThunkWithJumpTo` because it is already generated and then we return
> the same value we returned to `v1` before. Is the code installed during `v1`
> execution visible to `v2`?

I've got answers to these questions and they are explained on new ChangeLog entry.

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 364555 [details]
Patch

[Caio Lima]

Created attachment 364689 [details]
Patch

[Caio Lima]

Ping Review

[Keith Miller]

Comment on attachment 364689 [details]
Patch

r=me. Although, I wonder if the getters should only have a weak static reference to some refcounted thing and we regenerate when needed. It's probably not worth doing now though.

[Caio Lima]

Comment on attachment 364689 [details]
Patch

Thx a lot for the review! I created https://bugs.webkit.org/show_bug.cgi?id=195941 to investigate usage of weak references.

[WebKit Commit Bot]

Comment on attachment 364689 [details]
Patch

Clearing flags on attachment: 364689

Committed r243136: <https://trac.webkit.org/changeset/243136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49017418>