Bug 187886 - runJITThreadLimitTests is failing
Summary: runJITThreadLimitTests is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-21 19:07 PDT by Filip Pizlo
Modified: 2018-07-24 22:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.57 KB, patch)
2018-07-23 12:42 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-07-21 19:07:09 PDT
I am going to skip it.
Comment 1 Filip Pizlo 2018-07-21 19:08:17 PDT
Also, I think that configuring this by setting Options is not right, since it won't have an effect once any VM is started.  Callers can't guarantee that no VM has started prior to any of their calls to those functions.  I think it's better if those APIs talk to the Worklists directly, and shutdown/startup threads if they change the number of threads after the threads have started.
Comment 2 Tadeu Zagallo 2018-07-23 12:42:42 PDT
Created attachment 345596 [details]
Patch
Comment 3 Saam Barati 2018-07-23 12:46:18 PDT
Comment on attachment 345596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345596&action=review

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:538
> +    return numberOfDFGCompilerThreads ?: Options::numberOfDFGCompilerThreads();

style nit: We don't use shorthand ternary operator style wise in WebKit AFAIK. So:
return numberOfDFGCompilerThreads ? numberOfDFGCompilerThreads : Options::numberOfDFGCompilerThreads();

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:543
> +    return numberOfFTLCompilerThreads ?: Options::numberOfFTLCompilerThreads();

ditto
Comment 4 Filip Pizlo 2018-07-23 12:46:30 PDT
Comment on attachment 345596 [details]
Patch

This doesn't appear to tell the worklists to shut down threads if they have already started them.
Comment 5 Filip Pizlo 2018-07-23 12:47:33 PDT
(In reply to Saam Barati from comment #3)
> Comment on attachment 345596 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345596&action=review
> 
> > Source/JavaScriptCore/dfg/DFGWorklist.cpp:538
> > +    return numberOfDFGCompilerThreads ?: Options::numberOfDFGCompilerThreads();
> 
> style nit: We don't use shorthand ternary operator style wise in WebKit
> AFAIK. So:
> return numberOfDFGCompilerThreads ? numberOfDFGCompilerThreads :
> Options::numberOfDFGCompilerThreads();
> 
> > Source/JavaScriptCore/dfg/DFGWorklist.cpp:543
> > +    return numberOfFTLCompilerThreads ?: Options::numberOfFTLCompilerThreads();
> 
> ditto

I didn't know about this shorthand!  Any reason not to use it?  I think it's OK to adopt new language functionality if we know that all of the compilers we use support it.  I don't think we've ever explicitly said not to use this feature.
Comment 6 Tadeu Zagallo 2018-07-23 12:49:30 PDT
(In reply to Filip Pizlo from comment #4)
> Comment on attachment 345596 [details]
> Patch
> 
> This doesn't appear to tell the worklists to shut down threads if they have
> already started them.

That had already been handled in Workklist::setNumberOfThreads
Comment 7 Saam Barati 2018-07-23 13:04:20 PDT
(In reply to Filip Pizlo from comment #5)
> (In reply to Saam Barati from comment #3)
> > Comment on attachment 345596 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=345596&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGWorklist.cpp:538
> > > +    return numberOfDFGCompilerThreads ?: Options::numberOfDFGCompilerThreads();
> > 
> > style nit: We don't use shorthand ternary operator style wise in WebKit
> > AFAIK. So:
> > return numberOfDFGCompilerThreads ? numberOfDFGCompilerThreads :
> > Options::numberOfDFGCompilerThreads();
> > 
> > > Source/JavaScriptCore/dfg/DFGWorklist.cpp:543
> > > +    return numberOfFTLCompilerThreads ?: Options::numberOfFTLCompilerThreads();
> > 
> > ditto
> 
> I didn't know about this shorthand!  Any reason not to use it?  I think it's
> OK to adopt new language functionality if we know that all of the compilers
> we use support it.  I don't think we've ever explicitly said not to use this
> feature.

Seems like this is not standard C++, that would be the main reason we may want to stay away:

https://stackoverflow.com/questions/34679692/ternary-operator-without-second-operand

I'm not sure if we care if this is an extension or not though, if all our compilers support it...
Comment 8 Filip Pizlo 2018-07-24 08:33:55 PDT
Comment on attachment 345596 [details]
Patch

r=me too after Tadeu explained it to me.
Comment 9 WebKit Commit Bot 2018-07-24 16:42:30 PDT
Comment on attachment 345596 [details]
Patch

Clearing flags on attachment: 345596

Committed r234180: <https://trac.webkit.org/changeset/234180>
Comment 10 WebKit Commit Bot 2018-07-24 16:42:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-07-24 16:43:27 PDT
<rdar://problem/42561966>
Comment 12 Ross Kirsling 2018-07-24 21:23:12 PDT
(In reply to Saam Barati from comment #7)
> (In reply to Filip Pizlo from comment #5)
> > (In reply to Saam Barati from comment #3)
> > > Comment on attachment 345596 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=345596&action=review
> > > 
> > > > Source/JavaScriptCore/dfg/DFGWorklist.cpp:538
> > > > +    return numberOfDFGCompilerThreads ?: Options::numberOfDFGCompilerThreads();
> > > 
> > > style nit: We don't use shorthand ternary operator style wise in WebKit
> > > AFAIK. So:
> > > return numberOfDFGCompilerThreads ? numberOfDFGCompilerThreads :
> > > Options::numberOfDFGCompilerThreads();
> > > 
> > > > Source/JavaScriptCore/dfg/DFGWorklist.cpp:543
> > > > +    return numberOfFTLCompilerThreads ?: Options::numberOfFTLCompilerThreads();
> > > 
> > > ditto
> > 
> > I didn't know about this shorthand!  Any reason not to use it?  I think it's
> > OK to adopt new language functionality if we know that all of the compilers
> > we use support it.  I don't think we've ever explicitly said not to use this
> > feature.
> 
> Seems like this is not standard C++, that would be the main reason we may
> want to stay away:
> 
> https://stackoverflow.com/questions/34679692/ternary-operator-without-second-
> operand
> 
> I'm not sure if we care if this is an extension or not though, if all our
> compilers support it...

MSVC does not support this syntax, so this patch broke WinCairo:
https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Build%29/builds/2036/steps/compile-webkit/logs/stdio
Comment 13 Fujii Hironori 2018-07-24 22:30:03 PDT
Committed r234190: <https://trac.webkit.org/changeset/234190>