Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JSC] Extend megamorphic store cache with invalidating replacement watchpoint set #14001

Merged
merged 1 commit into from
May 18, 2023
Merged

[JSC] Extend megamorphic store cache with invalidating replacement watchpoint set #14001

merged 1 commit into from
May 18, 2023

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented May 18, 2023 •
edited by webkit-early-warning-system

b4d99e6

[JSC] Extend megamorphic store cache with invalidating replacement watchpoint set
https://bugs.webkit.org/show_bug.cgi?id=256943
rdar://109493502

Reviewed by Justin Michaud.

This patch recovers regression in JetStream2/typescript after introducing megamorphic store cache.
The reason is that the previous cache was a bit too conservative, and we are using slow path for
Structure::isWatchingReplacement case, but JetStream2/typescript includes legit use of this kind of
Structure with megamorphic cache.
This patch allows Structure::isWatchingReplacement case, and invalidate replacement watchpoint when
caching replace structure. This recovers and rather improves JetStream2/typescript.

* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::regenerate):
* Source/JavaScriptCore/jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::storeMegamorphicProperty):
* Source/JavaScriptCore/jit/JITOperations.cpp:
(JSC::putByIdMegamorphic):

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

386f3af

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

@Constellation Constellation self-assigned this May 18, 2023
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label May 18, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 0c2ac06)

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

@Constellation Constellation force-pushed the eng/JSC-Extend-megamorphic-store-cache-with-invalidating-replacement-watchpoint-set branch from 0c2ac06 to de701f7 Compare May 18, 2023 03:13
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash de701f7)

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

@Constellation Constellation force-pushed the eng/JSC-Extend-megamorphic-store-cache-with-invalidating-replacement-watchpoint-set branch from de701f7 to 0af1a3f Compare May 18, 2023 03:17
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 0af1a3f)

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

@Constellation Constellation force-pushed the eng/JSC-Extend-megamorphic-store-cache-with-invalidating-replacement-watchpoint-set branch from 0af1a3f to 386f3af Compare May 18, 2023 08:52
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 386f3af)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 18, 2023
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label May 18, 2023
@Constellation Constellation marked this pull request as ready for review May 18, 2023 16:34
@Constellation Constellation requested a review from a team as a code owner May 18, 2023 16:34
justinmichaud
justinmichaud approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@justinmichaud justinmichaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

justinmichaud
justinmichaud reviewed May 18, 2023
View reviewed changes
Source/JavaScriptCore/runtime/StructureInlines.h
didReplacePropertySlow(offset);
}

inline void Structure::didCachePropertyReplacement(VM& vm, PropertyOffset offset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this differ from didReplacePropertySlow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not annotated with EXPORT macro so that it can be only called from JSC framework and LTO inlines it.

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 18, 2023
@Constellation
[JSC] Extend megamorphic store cache with invalidating replacement wa…
b4d99e6 
…tchpoint set

https://bugs.webkit.org/show_bug.cgi?id=256943
rdar://109493502

Reviewed by Justin Michaud.

This patch recovers regression in JetStream2/typescript after introducing megamorphic store cache.
The reason is that the previous cache was a bit too conservative, and we are using slow path for
Structure::isWatchingReplacement case, but JetStream2/typescript includes legit use of this kind of
Structure with megamorphic cache.
This patch allows Structure::isWatchingReplacement case, and invalidate replacement watchpoint when
caching replace structure. This recovers and rather improves JetStream2/typescript.

* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::regenerate):
* Source/JavaScriptCore/jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::storeMegamorphicProperty):
* Source/JavaScriptCore/jit/JITOperations.cpp:
(JSC::putByIdMegamorphic):

Canonical link: https://commits.webkit.org/264212@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Extend-megamorphic-store-cache-with-invalidating-replacement-watchpoint-set branch from 386f3af to b4d99e6 Compare May 18, 2023 21:12
@webkit-commit-queue
Copy link
Collaborator

Committed 264212@main (b4d99e6): https://commits.webkit.org/264212@main

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

@webkit-commit-queue webkit-commit-queue merged commit b4d99e6 into WebKit:main May 18, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 18, 2023
@Constellation Constellation deleted the eng/JSC-Extend-megamorphic-store-cache-with-invalidating-replacement-watchpoint-set branch May 18, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmichaud justinmichaud justinmichaud approved these changes

Assignees

@Constellation Constellation

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
5 participants
@Constellation @webkit-early-warning-system @webkit-commit-queue @justinmichaud @webkit-ews-buildbot