195439 2019-03-07 15:56:00 -0800 [PlayStation] Increase the default pthread stack size 2024-04-19 08:39:20 -0700 1 1 1 Unclassified WebKit Platform WebKit Nightly Build Unspecified Unspecified NEW P2 Normal --- 1 chris.reid webkit-unassigned ahmad.saleem792 benjamin cdumez cmarcelo dbates don.olmstead ews-watchlist fujii mark.lam ysuzuki oldest_to_newest 1513762 0 chris.reid 2019-03-07 15:56:00 -0800 Our PlayStation platform has a very small stack size of 64 KB for new threads. With default jsc VM options, web workers would instantly stack overflow on a 64 KB stack. The main thread stack size is more reasonable at 2 MB. This has came up before with https://bugs.webkit.org/show_bug.cgi?id=40103. I tested with these jsc VM options: maxPerThreadStackUsage = 64 * KB softReservedZoneSize = 32 * KB reservedZoneSize = 16 * KB On linux with those options and `ulimit -s 64`, I ran javascriptcore-tests --memory-limited and there are still about 1,000 failures out of 55,000 tests. There's 67 failures using `ulimit -s 512` and default VM options. 512 KB seems to also match the secondary thread stack size on iOS and Mac. 1513785 1 363951 chris.reid 2019-03-07 16:18:37 -0800 Created attachment 363951 patch 1513786 2 ews-watchlist 2019-03-07 16:20:51 -0800 Attachment 363951 did not pass style-queue: ERROR: Source/WTF/wtf/posix/ThreadingPOSIX.cpp:214: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 1513801 3 363951 mark.lam 2019-03-07 16:35:01 -0800 Comment on attachment 363951 patch LGTM but someone with PlayStation knowledge should also review to confirm if 512K is a reasonable stack size to use. 1513839 4 don.olmstead 2019-03-07 17:43:05 -0800 (In reply to Mark Lam from comment #3) > Comment on attachment 363951 [details] > patch > > LGTM but someone with PlayStation knowledge should also review to confirm if > 512K is a reasonable stack size to use. I'm fine with the change. The thing I'm wondering is whether it would be good for WebKit to have a thread's stack size be more configurable in general during creation. 1513841 5 mark.lam 2019-03-07 17:49:49 -0800 (In reply to Don Olmstead from comment #4) > (In reply to Mark Lam from comment #3) > > Comment on attachment 363951 [details] > > patch > > > > LGTM but someone with PlayStation knowledge should also review to confirm if > > 512K is a reasonable stack size to use. > > I'm fine with the change. The thing I'm wondering is whether it would be > good for WebKit to have a thread's stack size be more configurable in > general during creation. See Options::maxPerThreadStackUsage(). For tests, we explicitly set this option to a smaller amount do that tests that overflow the stack can finish faster. However, this option is at the JSC level, not WTF. Actual allowed stack capacity is computed as min of Options::maxPerThreadStackUsage() and capacity reported by StackBounds. 1513843 6 mark.lam 2019-03-07 17:51:13 -0800 (In reply to Mark Lam from comment #5) > (In reply to Don Olmstead from comment #4) > > (In reply to Mark Lam from comment #3) > > > Comment on attachment 363951 [details] > > > patch > > > > > > LGTM but someone with PlayStation knowledge should also review to confirm if > > > 512K is a reasonable stack size to use. > > > > I'm fine with the change. The thing I'm wondering is whether it would be > > good for WebKit to have a thread's stack size be more configurable in > > general during creation. > > See Options::maxPerThreadStackUsage(). For tests, we explicitly set this > option to a smaller amount do that tests that overflow the stack can finish > faster. > > However, this option is at the JSC level, not WTF. Actual allowed stack > capacity is computed as min of Options::maxPerThreadStackUsage() and > capacity reported by StackBounds. Note: it's done this way because we can't always control what the OS allocates for stack capacity. We just make sure that we don't use more than either limits. 2029837 7 ahmad.saleem792 2024-04-19 08:39:20 -0700 Current code: https://searchfox.org/wubkat/rev/f8504f25d2ccc6a64957e53ab8c4e42885007fb0/Source/WTF/wtf/posix/ThreadingPOSIX.cpp#314 bool Thread::establishHandle(NewThreadContext* context, std::optional<size_t> stackSize, QOS qos, SchedulingPolicy schedulingPolicy) { pthread_t threadHandle; pthread_attr_t attr; pthread_attr_init(&attr); #if HAVE(QOS_CLASSES) pthread_attr_set_qos_class_np(&attr, dispatchQOSClass(qos), 0); #endif #if HAVE(SCHEDULING_POLICIES) pthread_attr_setschedpolicy(&attr, schedPolicy(schedulingPolicy)); #endif if (stackSize) pthread_attr_setstacksize(&attr, stackSize.value()); int error = pthread_create(&threadHandle, &attr, wtfThreadEntryPoint, context); pthread_attr_destroy(&attr); if (error) { LOG_ERROR("Failed to create pthread at entry point %p with context %p", wtfThreadEntryPoint, context); return false; } #if OS(LINUX) int policy = schedPolicy(qos, schedulingPolicy); if (policy == SCHED_RR) RealTimeThreads::singleton().registerThread(*this); else { struct sched_param param = { }; error = pthread_setschedparam(threadHandle, policy | SCHED_RESET_ON_FORK, &param); if (error) LOG_ERROR("Failed to set sched policy %d for thread %ld: %s", policy, threadHandle, safeStrerror(error).data()); } #else #if !HAVE(QOS_CLASSES) UNUSED_PARAM(qos); #endif #if !HAVE(SCHEDULING_POLICIES) UNUSED_PARAM(schedulingPolicy); #endif #endif establishPlatformSpecificHandle(threadHandle); return true; } ___ We don't seem to have anything specific for 'Playstation' platform like this patch was modifying: #if PLATFORM(PLAYSTATION) and searching via bug ID - '195439' and didn't see that this landed. CCing - Don & Fuji to check, if this is needed or we can close this. 363951 2019-03-07 16:18:37 -0800 2022-02-28 04:07:00 -0800 patch pthread.diff text/plain 1540 chris.reid ZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nIGIvU291cmNlL1dURi9DaGFuZ2VMb2cK aW5kZXggMWE5YjE5ZDcxMTkuLmU3ZGI1YTExYWI3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL0No YW5nZUxvZworKysgYi9Tb3VyY2UvV1RGL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE5 LTAzLTA3ICBDaHJpc3RvcGhlciBSZWlkICA8Y2hyaXMucmVpZEBzb255LmNvbT4KKworICAgICAg ICBbUGxheVN0YXRpb25dIEluY3JlYXNlIHRoZSBkZWZhdWx0IHB0aHJlYWQgc3RhY2sgc2l6ZQor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk1NDM5CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgT3VyIHBsYXRm b3JtIGhhcyBhIHZlcnkgc21hbGwgNjQgS0Igc2Vjb25kYXJ5IHRocmVhZCBzdGFjayBzaXplLgor ICAgICAgICBUaGlzIHNtYWxsIHN0YWNrIHNpemUgY2F1c2VzIHByb2JsZW1zIHJ1bm5pbmcgbW9z dCBXZWIgV29ya2VycyB3aXRoIGRlZmF1bHQganNjIFZNIG9wdGlvbnMuCisgICAgICAgIEluY3Jl YXNlIHRoZSBwdGhyZWFkX2NyZWF0ZSBzdGFjayBzaXplIHRvIDUxMiBLQiBtYXRjaGluZyBpT1Mg YW5kIE1hYy4KKworICAgICAgICAqIHd0Zi9wb3NpeC9UaHJlYWRpbmdQT1NJWC5jcHA6CisKIDIw MTktMDItMjggIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgQWRk IFdlYWtIYXNoU2V0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9wb3NpeC9UaHJlYWRpbmdQ T1NJWC5jcHAgYi9Tb3VyY2UvV1RGL3d0Zi9wb3NpeC9UaHJlYWRpbmdQT1NJWC5jcHAKaW5kZXgg NjRjZDFmNzE4ZDUuLmNiMjk3Y2JlMzExIDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9wb3Np eC9UaHJlYWRpbmdQT1NJWC5jcHAKKysrIGIvU291cmNlL1dURi93dGYvcG9zaXgvVGhyZWFkaW5n UE9TSVguY3BwCkBAIC0yMDgsNiArMjA4LDExIEBAIGJvb2wgVGhyZWFkOjplc3RhYmxpc2hIYW5k bGUoTmV3VGhyZWFkQ29udGV4dCogY29udGV4dCkKICAgICBwdGhyZWFkX2F0dHJfaW5pdCgmYXR0 cik7CiAjaWYgSEFWRShRT1NfQ0xBU1NFUykKICAgICBwdGhyZWFkX2F0dHJfc2V0X3Fvc19jbGFz c19ucCgmYXR0ciwgYWRqdXN0ZWRRT1NDbGFzcyhRT1NfQ0xBU1NfVVNFUl9JTklUSUFURUQpLCAw KTsKKyNlbmRpZgorI2lmIFBMQVRGT1JNKFBMQVlTVEFUSU9OKQorICAgIHNpemVfdCBkZWZhdWx0 U3RhY2tTaXplOworICAgIGlmIChwdGhyZWFkX2F0dHJfZ2V0c3RhY2tzaXplKCZhdHRyLCAmZGVm YXVsdFN0YWNrU2l6ZSkgPT0gMCkKKyAgICAgICAgcHRocmVhZF9hdHRyX3NldHN0YWNrc2l6ZSgm YXR0ciwgc3RkOjptYXgoNTEyICogS0IsIGRlZmF1bHRTdGFja1NpemUpKTsKICNlbmRpZgogICAg IGludCBlcnJvciA9IHB0aHJlYWRfY3JlYXRlKCZ0aHJlYWRIYW5kbGUsICZhdHRyLCB3dGZUaHJl YWRFbnRyeVBvaW50LCBjb250ZXh0KTsKICAgICBwdGhyZWFkX2F0dHJfZGVzdHJveSgmYXR0cik7 Cg==