Summary: | [JSC] LLIntEntryPoint creates same DirectJITCode for all functions | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 193606 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Caio Lima
2019-02-14 03:48:10 PST
Created attachment 362008 [details]
WIP - Patch
This is just the initial idea. I want to see how bots handle it.
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. 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
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. 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
Created attachment 362036 [details]
WIP - Patch
Testing things on EWS
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.
(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. (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. 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.
(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) It is counter-intuitive, but according to my measuring using massif, the version with this patch is actually taking more memory than baseline version... 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 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
Created attachment 362306 [details]
WIP - Patch
Created attachment 362313 [details]
WIP - Patch
Lets see EWS.
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.
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?
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`? Created attachment 362897 [details]
Patch
(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. Ping Review Created attachment 364555 [details]
Patch
Created attachment 364689 [details]
Patch
Ping Review 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.
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. Comment on attachment 364689 [details] Patch Clearing flags on attachment: 364689 Committed r243136: <https://trac.webkit.org/changeset/243136> All reviewed patches have been landed. Closing bug. |