Bug 195439 – [PlayStation] Increase the default pthread stack size

• Status: NEW
• Component: Platform

[Christopher Reid]

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.

[Christopher Reid]

Created attachment 363951 [details]
patch

[EWS Watchlist]

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.

[Mark Lam]

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.

[Don Olmstead]

(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.

[Mark Lam]

(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.

[Mark Lam]

(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.

[Ahmad Saleem]

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.