Summary: | [PlayStation] Increase the default pthread stack size | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Christopher Reid <chris.reid> | ||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | ahmad.saleem792, benjamin, cdumez, cmarcelo, dbates, don.olmstead, ews-watchlist, Hironori.Fujii, mark.lam, ysuzuki | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Christopher Reid
2019-03-07 15:56:00 PST
Created attachment 363951 [details]
patch
Attachment 363951 [details] 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.
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.
(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. (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. (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. 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, ¶m); 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. |