Bug 104406 – JSC should scale the optimization threshold for a code block according to the cost of compiling it

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 178289 [details]
the patch

[WebKit Review Bot]

Attachment 178289 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGCapabilities.h:42:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGCapabilities.h:43:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGCapabilities.h:44:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGCapabilities.h:45:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGCapabilities.h:46:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGCapabilities.h:47:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 178289 [details]
the patch

Attachment 178289 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15173983

[Geoffrey Garen]

Windows export file needs some babysitting:


6>CodeBlock.obj : error LNK2019: unresolved external symbol "public: int __thiscall JSC::CodeBlock::counterValueForOptimizeAfterWarmUp(void)" (?counterValueForOptimizeAfterWarmUp@CodeBlock@JSC@@QAEHXZ) referenced in function "public: void __thiscall JSC::CodeBlock::optimizeAfterWarmUp(void)" (?optimizeAfterWarmUp@CodeBlock@JSC@@QAEXXZ)

[Geoffrey Garen]

Comment on attachment 178289 [details]
the patch

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

r=me, please fix comments before landing.

> Source/JavaScriptCore/dfg/DFGCapabilities.h:48
> +bool mightCompileEval(CodeBlock* codeBlock);
> +bool mightCompileProgram(CodeBlock* codeBlock);
> +bool mightCompileFunctionForCall(CodeBlock* codeBlock);
> +bool mightCompileFunctionForConstruct(CodeBlock* codeBlock);
> +bool mightInlineFunctionForCall(CodeBlock* codeBlock);
> +bool mightInlineFunctionForConstruct(CodeBlock* codeBlock);
>  

Style bot is right here, for once.

[Filip Pizlo]

(In reply to comment #4)
> Windows export file needs some babysitting:
> 
> 
> 6>CodeBlock.obj : error LNK2019: unresolved external symbol "public: int __thiscall JSC::CodeBlock::counterValueForOptimizeAfterWarmUp(void)" (?counterValueForOptimizeAfterWarmUp@CodeBlock@JSC@@QAEHXZ) referenced in function "public: void __thiscall JSC::CodeBlock::optimizeAfterWarmUp(void)" (?optimizeAfterWarmUp@CodeBlock@JSC@@QAEXXZ)

I think it's once again misuse of #ifdef's like ENABLE(DFG.. and so on. :-/  I'm trying to fiddle with it now.

[Filip Pizlo]

(In reply to comment #5)
> (From update of attachment 178289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178289&action=review
> 
> r=me, please fix comments before landing.
> 
> > Source/JavaScriptCore/dfg/DFGCapabilities.h:48
> > +bool mightCompileEval(CodeBlock* codeBlock);
> > +bool mightCompileProgram(CodeBlock* codeBlock);
> > +bool mightCompileFunctionForCall(CodeBlock* codeBlock);
> > +bool mightCompileFunctionForConstruct(CodeBlock* codeBlock);
> > +bool mightInlineFunctionForCall(CodeBlock* codeBlock);
> > +bool mightInlineFunctionForConstruct(CodeBlock* codeBlock);
> >  
> 
> Style bot is right here, for once.

Yup, I fixed.

[Filip Pizlo]

Created attachment 178303 [details]
work in progress

This should make EWS happy.

[Filip Pizlo]

Comment on attachment 178303 [details]
work in progress

This causes a massive regression on Kraken audio benchmarks.  The regression only shows up in browser.  Investigating the reasons now.

[Filip Pizlo]

It looks like what is going on is that we've got some code blocks in the Kraken audio benchmarks that rely heavily on OSR exit profiling.  Our previous threshold for triggering recompilation was 5000 (thresholdForOptimizeAfterLongWarmUp); this code effectively increases that threshold since we have scaling by instruction count and most code blocks that benefit from recompilation tend to be large-ish.  

So, I experimented with lowering the thresholdForOptimizeAfterLongWarmUp.  With 3000, we already neutralize the slow-down on Kraken.  But lowering it further seems to help even more.  It looks like setting it to 1000 helps the most.  Here are the performance numbers on various benchmarks for 1000, 2000, and 3000:


3000:
jsc:
   0.7% faster on SunSpider
   neutral on V8v7
   ~1% slower on Kraken
drt:
   1% faster on SunSpider
   ~1% faster on V8v7
   neutral on Octane
   2% slower on Kraken
   ~10% faster on JSBench
   1.5% faster on DSP

2000:
jsc:
   0.9% faster on SunSpider
   neutral on V8v7
   0.9% slower on Kraken
drt:
   neutral on SunSpider
   0.6% faster on V8v7
   ~2% slower on Kraken
   ~7.8% faster on JSBench
   1.9% faster on DSP

1000:
jsc:
   1.4% faster on SunSpider
   0.4% slower on V8v7
   0.6% slower on Kraken
drt:
   0.6% faster on SunSpider
   1.3% faster on V8v7
   neutral on Kraken
   1% faster on JSBench
   neutral on DSP

Further tests seem to imply that the speed-up on SunSpider in DRT is closer to 1% with 1000, but I can't be sure.  Anyway, 1000 seems best since I'd rather be neutral on Kraken and get a 1% V8v7 & SunSpider speed-up.

[Filip Pizlo]

Created attachment 178396 [details]
the patch

I fixed all regressions.  It took some effort.

[WebKit Review Bot]

Attachment 178396 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/bytecode/CodeBlock.cpp:3110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:3115:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:3125:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 178397 [details]
the patch

fix style

[Oliver Hunt]

Comment on attachment 178397 [details]
the patch

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

r=me

> Source/JavaScriptCore/profiler/ProfilerDatabase.cpp:61
> +    Bytecodes* result = addBytecodes(
> +        codeBlock->hash(), codeBlock->sourceCodeForTools());

something this short could just be a single line :p

[Filip Pizlo]

(In reply to comment #14)
> (From update of attachment 178397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178397&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/profiler/ProfilerDatabase.cpp:61
> > +    Bytecodes* result = addBytecodes(
> > +        codeBlock->hash(), codeBlock->sourceCodeForTools());
> 
> something this short could just be a single line :p

You make a fair point!  Fixed.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/137094

[Geoffrey Garen]

Should we reconsider these limits, now that bug 104500 has been discovered and fixed?