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] Skip notifyOne when all JIT threads are running #24185

Merged
merged 1 commit into from
Feb 10, 2024
Merged

[JSC] Skip notifyOne when all JIT threads are running #24185

merged 1 commit into from
Feb 10, 2024

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Feb 10, 2024 •
edited by webkit-early-warning-system
Loading

17e76f5

[JSC] Skip notifyOne when all JIT threads are running
https://bugs.webkit.org/show_bug.cgi?id=269111
rdar://122677279

Reviewed by Mark Lam.

Let's avoid calling notifyOne when all JIT threads are currently running.
In that case, they will pick the enqueued plan without notifying anyway.
This can skip some of costly syscalls like pthread_condvar related ones.
We also change JITWorklist::suspendAllThreads to first use tryLock for all threads.
So then, we can eagerly suspend currently-not-running-threads. And after that,
we eventually ensure all threads are not running. This avoids starting JIT compilation
in the latter thread while it was not having that when JITWorklist::suspendAllThreads started.

* Source/JavaScriptCore/jit/JITWorklist.cpp:
(JSC::JITWorklist::JITWorklist):
(JSC::JITWorklist::enqueue):
(JSC::JITWorklist::removeDeadPlans):
(JSC::JITWorklist::visitWeakReferences):
* Source/JavaScriptCore/jit/JITWorklist.h:
* Source/JavaScriptCore/jit/JITWorklistThread.cpp:
(JSC::JITWorklistThread::work):
* Source/JavaScriptCore/jit/JITWorklistThread.h:

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

bb608e0

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 ✅ 🛠 gtk
  🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
  🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@Constellation Constellation requested a review from a team as a code owner February 10, 2024 00:20
@Constellation Constellation self-assigned this Feb 10, 2024
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Feb 10, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 10, 2024 •
edited
Loading

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

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 ✅ 🛠 gtk
  🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
  🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

MenloDorian
MenloDorian approved these changes Feb 10, 2024
View reviewed changes
Copy link

@MenloDorian MenloDorian 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

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 10, 2024
@Constellation
[JSC] Skip notifyOne when all JIT threads are running
17e76f5 
https://bugs.webkit.org/show_bug.cgi?id=269111
rdar://122677279

Reviewed by Mark Lam.

Let's avoid calling notifyOne when all JIT threads are currently running.
In that case, they will pick the enqueued plan without notifying anyway.
This can skip some of costly syscalls like pthread_condvar related ones.
We also change JITWorklist::suspendAllThreads to first use tryLock for all threads.
So then, we can eagerly suspend currently-not-running-threads. And after that,
we eventually ensure all threads are not running. This avoids starting JIT compilation
in the latter thread while it was not having that when JITWorklist::suspendAllThreads started.

* Source/JavaScriptCore/jit/JITWorklist.cpp:
(JSC::JITWorklist::JITWorklist):
(JSC::JITWorklist::enqueue):
(JSC::JITWorklist::removeDeadPlans):
(JSC::JITWorklist::visitWeakReferences):
* Source/JavaScriptCore/jit/JITWorklist.h:
* Source/JavaScriptCore/jit/JITWorklistThread.cpp:
(JSC::JITWorklistThread::work):
* Source/JavaScriptCore/jit/JITWorklistThread.h:

Canonical link: https://commits.webkit.org/274407@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Skip-notifyOne-when-all-JIT-threads-are-running branch from bb608e0 to 17e76f5 Compare February 10, 2024 03:43
@webkit-commit-queue
Copy link
Collaborator

Committed 274407@main (17e76f5): https://commits.webkit.org/274407@main

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

@webkit-commit-queue webkit-commit-queue merged commit 17e76f5 into WebKit:main Feb 10, 2024
@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 Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MenloDorian MenloDorian MenloDorian 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
4 participants
@Constellation @webkit-early-warning-system @webkit-commit-queue @MenloDorian