Bug 187886 – runJITThreadLimitTests is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

I am going to skip it.

[Filip Pizlo]

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.

[Tadeu Zagallo]

Created attachment 345596 [details]
Patch

[Saam Barati]

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

[Filip Pizlo]

Comment on attachment 345596 [details]
Patch

This doesn't appear to tell the worklists to shut down threads if they have already started them.

[Filip Pizlo]

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

[Tadeu Zagallo]

(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

[Saam Barati]

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

[Filip Pizlo]

Comment on attachment 345596 [details]
Patch

r=me too after Tadeu explained it to me.

[WebKit Commit Bot]

Comment on attachment 345596 [details]
Patch

Clearing flags on attachment: 345596

Committed r234180: <https://trac.webkit.org/changeset/234180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42561966>

[Ross Kirsling]

(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

[Fujii Hironori]

Committed r234190: <https://trac.webkit.org/changeset/234190>