[JSC] Reland Handler IC #28106

Constellation · 2024-05-03T16:53:05Z

`13455c7`

[JSC] Reland Handler IC
https://bugs.webkit.org/show_bug.cgi?id=273693
rdar://127496851

Reviewed by Keith Miller.

This patch enables limited variant of Handler IC. The limitation means,

1. Only enabled for Baseline JIT.
2. Getter and Setter are not supported yet.
3. We are caching entire code as an one handler. This is not the final form we would like to have.
Next step is splitting them into one per AccessCase and chain them.
4. After (3) gets done, we would like to put more data into InlineCacheHandler itself so that code
can be more and more sharable.

But even with this limited form, we are already observing good cache hit rate. So we take an approach starting with this,
and further extending Handler IC based on the above milestones.

We enable Handler IC, which is only enabled for Baseline JIT right now.
The IC is hash-consed via SharedJITStubSet. And InlineCacheCompiler first search for an already compiled stub, if it finds it,
we register watchpoint to this stub and use it without new compilation. If it is not found, we compile a new stub and register it to this table if possible.
When nobody uses this stub, then refCount becomes zero, and it automatically unregister itself from the table.
Each StructureStubInfo site's access cases is always subsumes stub's access cases. So GC will check validity via this StructureStubInfo's access cases, and
drop stub when it is no longer valid (as the same to the current IC).

The major difference from the last patch is adding multiple owners to the shared JIT IC.
Stubs are unregistering themselves from SharedJITStubSet when it reaches to zero-ref-count.
But this does not work well since CodeBlock can be destroyed lazily: if CodeBlock is dead, during that,
no GC scanning happens to this stub while ref-count is still non-zero.
For normal JIT stubs, we have owner concept for JIT stub. At GC end phase, we iterate stubs and check whether the owner is already dead.
And if it is dead, we mark it m_ownerIsDead. But shared JIT stub does not have this concept.
Instead, we have HashCountedSet for shared JIT stub, and register multiple owners for shared JIT stub. And at GC end phase, we scan owners
to wipe dead owners, and when live owners become zero, we mark the stub as m_ownerIsDead too.

* Source/JavaScriptCore/bytecode/AccessCase.cpp:
(JSC::AccessCase::tryGetAlternateBaseImpl const):
(JSC::AccessCase::canBeShared):
(JSC::AccessCase::tryGetAlternateBase const):
(JSC::AccessCase::hasAlternateBaseImpl const): Deleted.
(JSC::AccessCase::alternateBaseImpl const): Deleted.
(JSC::AccessCase::hasAlternateBase const): Deleted.
(JSC::AccessCase::alternateBase const): Deleted.
* Source/JavaScriptCore/bytecode/AccessCase.h:
(JSC::AccessCase::identifier const):
(JSC::AccessCase::dumpImpl const):
(JSC::AccessCase::updateIdentifier): Deleted.
* Source/JavaScriptCore/bytecode/GetByStatus.cpp:
(JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
* Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:
(JSC::GetterSetterAccessCase::tryGetAlternateBaseImpl const):
(JSC::GetterSetterAccessCase::hasAlternateBaseImpl const): Deleted.
(JSC::GetterSetterAccessCase::alternateBaseImpl const): Deleted.
* Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:
* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::generateImpl):
(JSC::InlineCacheCompiler::regenerate):
(JSC::isMegamorphicById): Deleted.
* Source/JavaScriptCore/bytecode/PutByStatus.cpp:
(JSC::PutByStatus::computeForStubInfo):
* Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h:
(JSC::PolymorphicAccessJITStubRoutine::isStillValid const):
* Source/JavaScriptCore/runtime/StructureID.h:

Canonical link: https://commits.webkit.org/278369@main

b6b9842

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch		✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

webkit-early-warning-system · 2024-05-03T16:53:11Z

EWS run on previous version of this PR (hash 30465a9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	~~🛠 wpe~~	~~🛠 wincairo~~
	✅ 🛠 ios-sim	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-skia
~~🛠 🧪 jsc~~	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	~~🛠 gtk~~
~~🛠 🧪 jsc-arm64~~	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~	~~🧪 api-gtk~~
	~~🛠 watch~~		~~🛠 jsc-armv7~~
	~~🛠 watch-sim~~		~~🧪 jsc-armv7-tests~~

webkit-early-warning-system · 2024-05-03T16:57:38Z

EWS run on previous version of this PR (hash 70615bc)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	~~🛠 wpe~~	~~🛠 wincairo~~
	~~🛠 ios-sim~~	❌ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	❌ 🧪 api-mac	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	❌ 🧪 mac-wk1	~~🛠 wpe-skia~~
~~🛠 🧪 jsc~~	~~🧪 api-ios~~	❌ 🧪 mac-wk2	❌ 🛠 gtk
~~🛠 🧪 jsc-arm64~~	~~🛠 tv~~	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	~~🛠 tv-sim~~	❌ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	~~🛠 watch~~		❌ 🛠 jsc-armv7
	~~🛠 watch-sim~~		❌ 🧪 jsc-armv7-tests

webkit-early-warning-system · 2024-05-03T17:07:57Z

EWS run on previous version of this PR (hash 8e0d558)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
✅ 🛠 🧪 jsc	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
	✅ 🛠 watch		✅ 🛠 jsc-armv7
	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

kmiller68

r=me with nits.

kmiller68 · 2024-05-03T18:22:39Z

Source/WTF/wtf/HashCountedSet.h

+    template<typename Functor>
+    bool removeAllIf(const Functor&);


Nit: could be const auto&

kmiller68 · 2024-05-03T19:24:05Z

Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp

@@ -84,6 +84,27 @@ IGNORE_GCC_WARNINGS_BEGIN("sequence-point")
 IGNORE_GCC_WARNINGS_END
 }

+bool GCAwareJITStubRoutine::removeDeadOwners(VM& vm)
+{
+    if (m_owner)


Can we add: ASSERT(vm.heap.isInPhase(CollectorPhase::End)); here.

webkit-early-warning-system · 2024-05-03T19:36:31Z

EWS run on current version of this PR (hash b6b9842)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch		✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

Constellation · 2024-05-04T06:55:28Z

I've searched fast/webgpu/texture-supports-blending.html in the history data, and it is crashing exact same reason occasionally without this change. So it is flaky tests. And this is the signal that WebCore DOM object is not correctly keeping wrapper alive (and then wrapper gets collected, causing semantic issue). So landing.
https://build.webkit.org/#/builders/933/builds/1463
https://build.webkit.org/#/builders/934/builds/2534
https://build.webkit.org/#/builders/934/builds/2525
https://build.webkit.org/#/builders/934/builds/2521

webkit-commit-queue · 2024-05-04T06:57:14Z

Failed to access https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json, blocking PR #28106

https://bugs.webkit.org/show_bug.cgi?id=273693 rdar://127496851 Reviewed by Keith Miller. This patch enables limited variant of Handler IC. The limitation means, 1. Only enabled for Baseline JIT. 2. Getter and Setter are not supported yet. 3. We are caching entire code as an one handler. This is not the final form we would like to have. Next step is splitting them into one per AccessCase and chain them. 4. After (3) gets done, we would like to put more data into InlineCacheHandler itself so that code can be more and more sharable. But even with this limited form, we are already observing good cache hit rate. So we take an approach starting with this, and further extending Handler IC based on the above milestones. We enable Handler IC, which is only enabled for Baseline JIT right now. The IC is hash-consed via SharedJITStubSet. And InlineCacheCompiler first search for an already compiled stub, if it finds it, we register watchpoint to this stub and use it without new compilation. If it is not found, we compile a new stub and register it to this table if possible. When nobody uses this stub, then refCount becomes zero, and it automatically unregister itself from the table. Each StructureStubInfo site's access cases is always subsumes stub's access cases. So GC will check validity via this StructureStubInfo's access cases, and drop stub when it is no longer valid (as the same to the current IC). The major difference from the last patch is adding multiple owners to the shared JIT IC. Stubs are unregistering themselves from SharedJITStubSet when it reaches to zero-ref-count. But this does not work well since CodeBlock can be destroyed lazily: if CodeBlock is dead, during that, no GC scanning happens to this stub while ref-count is still non-zero. For normal JIT stubs, we have owner concept for JIT stub. At GC end phase, we iterate stubs and check whether the owner is already dead. And if it is dead, we mark it m_ownerIsDead. But shared JIT stub does not have this concept. Instead, we have HashCountedSet for shared JIT stub, and register multiple owners for shared JIT stub. And at GC end phase, we scan owners to wipe dead owners, and when live owners become zero, we mark the stub as m_ownerIsDead too. * Source/JavaScriptCore/bytecode/AccessCase.cpp: (JSC::AccessCase::tryGetAlternateBaseImpl const): (JSC::AccessCase::canBeShared): (JSC::AccessCase::tryGetAlternateBase const): (JSC::AccessCase::hasAlternateBaseImpl const): Deleted. (JSC::AccessCase::alternateBaseImpl const): Deleted. (JSC::AccessCase::hasAlternateBase const): Deleted. (JSC::AccessCase::alternateBase const): Deleted. * Source/JavaScriptCore/bytecode/AccessCase.h: (JSC::AccessCase::identifier const): (JSC::AccessCase::dumpImpl const): (JSC::AccessCase::updateIdentifier): Deleted. * Source/JavaScriptCore/bytecode/GetByStatus.cpp: (JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback): * Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp: (JSC::GetterSetterAccessCase::tryGetAlternateBaseImpl const): (JSC::GetterSetterAccessCase::hasAlternateBaseImpl const): Deleted. (JSC::GetterSetterAccessCase::alternateBaseImpl const): Deleted. * Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h: * Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp: (JSC::InlineCacheCompiler::generateImpl): (JSC::InlineCacheCompiler::regenerate): (JSC::isMegamorphicById): Deleted. * Source/JavaScriptCore/bytecode/PutByStatus.cpp: (JSC::PutByStatus::computeForStubInfo): * Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h: (JSC::PolymorphicAccessJITStubRoutine::isStillValid const): * Source/JavaScriptCore/runtime/StructureID.h: Canonical link: https://commits.webkit.org/278369@main

webkit-commit-queue · 2024-05-04T07:04:49Z

Committed 278369@main (13455c7): https://commits.webkit.org/278369@main

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

Constellation self-assigned this May 3, 2024

Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label May 3, 2024

Constellation force-pushed the eng/JSC-Reland-Handler-IC branch from 30465a9 to 70615bc Compare May 3, 2024 16:57

Constellation marked this pull request as ready for review May 3, 2024 16:57

Constellation requested a review from a team as a code owner May 3, 2024 16:57

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 3, 2024

Constellation removed the merging-blocked Applied to prevent a change from being merged label May 3, 2024

Constellation force-pushed the eng/JSC-Reland-Handler-IC branch from 70615bc to 8e0d558 Compare May 3, 2024 17:07

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 3, 2024

kmiller68 approved these changes May 3, 2024

View reviewed changes

Constellation removed the merging-blocked Applied to prevent a change from being merged label May 3, 2024

Constellation force-pushed the eng/JSC-Reland-Handler-IC branch from 8e0d558 to b6b9842 Compare May 3, 2024 19:36

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 4, 2024

Constellation added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels May 4, 2024

webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels May 4, 2024

Constellation added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels May 4, 2024

webkit-commit-queue force-pushed the eng/JSC-Reland-Handler-IC branch from b6b9842 to 13455c7 Compare May 4, 2024 07:04

webkit-commit-queue merged commit 13455c7 into WebKit:main May 4, 2024

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 4, 2024

Constellation deleted the eng/JSC-Reland-Handler-IC branch May 4, 2024 07:55

This was referenced May 8, 2024

Unknown crash during run oven-sh/bun#10871

Closed

Segmentation fault oven-sh/bun#10992

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JSC] Reland Handler IC #28106

[JSC] Reland Handler IC #28106

Constellation commented May 3, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 3, 2024 •

edited

webkit-early-warning-system commented May 3, 2024 •

edited

webkit-early-warning-system commented May 3, 2024 •

edited

kmiller68 left a comment

kmiller68 May 3, 2024

Constellation May 3, 2024

kmiller68 May 3, 2024

Constellation May 3, 2024

webkit-early-warning-system commented May 3, 2024 •

edited

Constellation commented May 4, 2024

webkit-commit-queue commented May 4, 2024

webkit-commit-queue commented May 4, 2024

[JSC] Reland Handler IC #28106

[JSC] Reland Handler IC #28106

Conversation

Constellation commented May 3, 2024 • edited by webkit-early-warning-system

13455c7

webkit-early-warning-system commented May 3, 2024 • edited

webkit-early-warning-system commented May 3, 2024 • edited

webkit-early-warning-system commented May 3, 2024 • edited

kmiller68 left a comment

Choose a reason for hiding this comment

kmiller68 May 3, 2024

Choose a reason for hiding this comment

Constellation May 3, 2024

Choose a reason for hiding this comment

kmiller68 May 3, 2024

Choose a reason for hiding this comment

Constellation May 3, 2024

Choose a reason for hiding this comment

webkit-early-warning-system commented May 3, 2024 • edited

Constellation commented May 4, 2024

webkit-commit-queue commented May 4, 2024

webkit-commit-queue commented May 4, 2024

Constellation commented May 3, 2024 •

edited by webkit-early-warning-system

`13455c7`

webkit-early-warning-system commented May 3, 2024 •

edited

webkit-early-warning-system commented May 3, 2024 •

edited

webkit-early-warning-system commented May 3, 2024 •

edited

webkit-early-warning-system commented May 3, 2024 •

edited