I am going to skip it.
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.
Created attachment 345596 [details] Patch
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 on attachment 345596 [details] Patch This doesn't appear to tell the worklists to shut down threads if they have already started them.
(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.
(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
(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 on attachment 345596 [details] Patch r=me too after Tadeu explained it to me.
Comment on attachment 345596 [details] Patch Clearing flags on attachment: 345596 Committed r234180: <https://trac.webkit.org/changeset/234180>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42561966>
(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
Committed r234190: <https://trac.webkit.org/changeset/234190>