Skip to content

Enable JIT and FTL by default on the Windows Port#32972

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
iangrunert:ig/reenable-jit-by-default-win
Oct 2, 2024
Merged

Enable JIT and FTL by default on the Windows Port#32972
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
iangrunert:ig/reenable-jit-by-default-win

Conversation

@iangrunert
Copy link
Contributor

@iangrunert iangrunert commented Aug 31, 2024

847f97e

Enable JIT and FTL by default on the Windows Port
https://bugs.webkit.org/show_bug.cgi?id=278878

Reviewed by Yusuke Suzuki.

CodeBlock is 8 bytes larger on Windows - upping the static_assert.

Bumping the stack size for the main process from the Windows default 1MB
up to 8MB to match Mac and Linux, which fixes a stack overflow in test
JSTests/stress/object-create-will-do-transition.js

get-array-length-concurrently-change-mode.js adds an explicit time check
as the watchdog isn't firing. Likely a bug / problem in RunLoopWin if
this works on other platforms.

Making PrintStream printInternal add the 0x prefix to pointers on
Windows, fixing dfg-ai-direct-get-by-id-attribute-change-transition.js

There are still some test failures in JSC stress tests, but with the
exception of wasm/gc/array_new_fixed_long.js the remaining failures are
unrelated to JIT.

* JSTests/stress/get-array-length-concurrently-change-mode.js:
(main):
* Source/JavaScriptCore/bytecode/CodeBlock.h:
* Source/WTF/wtf/PrintStream.cpp:
(WTF::printInternal):
* Source/cmake/OptionsMSVC.cmake:
* Source/cmake/WebKitFeatures.cmake:

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

8265788

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

@iangrunert iangrunert requested review from a team and mcatanzaro as code owners August 31, 2024 00:45
Copy link
Member

Choose a reason for hiding this comment

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

This change looks strange as using return does not make it tail-call (since it is not a strict mode code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - this test was failing because Window's main thread stack size was only 1MB so it'd overflow the call stack well and truly before the other platforms.

Not 100% sure why adding the return caused it to not stack overflow, I'm surprised that would've changed the number of call frames or size of the call frames.

Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the Windows' output instead of changing the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - I still have to address this, I didn't notice I hadn't fully reverted my changes to this test file before pushing.

Copy link
Contributor

Choose a reason for hiding this comment

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

This code was added by me to disable JIT for Windows.
#28269
Can we just remove this code?

@Ahmad-S792 Ahmad-S792 added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Sep 3, 2024
@iangrunert iangrunert force-pushed the ig/reenable-jit-by-default-win branch from f3d58df to 472cabf Compare October 2, 2024 00:13
@iangrunert iangrunert force-pushed the ig/reenable-jit-by-default-win branch from 472cabf to 8265788 Compare October 2, 2024 01:17
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does Windows port need such a big 8MB stack? Other port doesn't seem to use such a big stack.

static std::optional<size_t> stackSize(ThreadType threadType)
{
// Return the stack size for the created thread based on its type.
// If the stack size is not specified, then use the system default. Platforms can tune the values here.
// Enable STACK_STATS in StackStats.h to create a build that will track the information for tuning.
#if PLATFORM(PLAYSTATION)
if (threadType == ThreadType::JavaScript)
return 512 * KB;
#elif OS(DARWIN) && (ASAN_ENABLED || ASSERT_ENABLED)
if (threadType == ThreadType::Compiler)
return 1 * MB; // ASan / Debug build needs more stack space.
#elif OS(WINDOWS)
// WebGL conformance tests need more stack space <https://webkit.org/b/261297>
if (threadType == ThreadType::Graphics)
#if defined(NDEBUG)
return 2 * MB;
#else
return 4 * MB;
#endif
#else
UNUSED_PARAM(threadType);
#endif
#if defined(DEFAULT_THREAD_STACK_SIZE_IN_KB) && DEFAULT_THREAD_STACK_SIZE_IN_KB > 0
return DEFAULT_THREAD_STACK_SIZE_IN_KB * 1024;
#elif OS(LINUX) && !defined(__BIONIC__) && !defined(__GLIBC__)
// on libcs other than glibc and bionic (e.g. musl) we are either unsure how big
// the default thread stack is, or we know it's too small - pick a robust default
return 1 * MB;
#else
// Use the platform's default stack size
return std::nullopt;
#endif
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a larger stack for the main thread - the JSTests/stress/object-create-will-do-transition.js test was overflowing the call stack. Mac and Linux both have 8MB stack size for the main thread, but with a 1MB stack size on Windows we're overflowing before we get close to the 1.5MB maxPerThreadStackUsage set for jsc stress tests.

We could update Threading.cpp#stackSize to keep a 1MB stack for other threads, we only need the increased stack size on the main thread.

@fujii fujii added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=278878

Reviewed by Yusuke Suzuki.

CodeBlock is 8 bytes larger on Windows - upping the static_assert.

Bumping the stack size for the main process from the Windows default 1MB
up to 8MB to match Mac and Linux, which fixes a stack overflow in test
JSTests/stress/object-create-will-do-transition.js

get-array-length-concurrently-change-mode.js adds an explicit time check
as the watchdog isn't firing. Likely a bug / problem in RunLoopWin if
this works on other platforms.

Making PrintStream printInternal add the 0x prefix to pointers on
Windows, fixing dfg-ai-direct-get-by-id-attribute-change-transition.js

There are still some test failures in JSC stress tests, but with the
exception of wasm/gc/array_new_fixed_long.js the remaining failures are
unrelated to JIT.

* JSTests/stress/get-array-length-concurrently-change-mode.js:
(main):
* Source/JavaScriptCore/bytecode/CodeBlock.h:
* Source/WTF/wtf/PrintStream.cpp:
(WTF::printInternal):
* Source/cmake/OptionsMSVC.cmake:
* Source/cmake/WebKitFeatures.cmake:

Canonical link: https://commits.webkit.org/284574@main
@webkit-commit-queue webkit-commit-queue force-pushed the ig/reenable-jit-by-default-win branch from 8265788 to 847f97e Compare October 2, 2024 19:51
@webkit-commit-queue
Copy link
Collaborator

Committed 284574@main (847f97e): https://commits.webkit.org/284574@main

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

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

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants