Bug 68116 – Tiered compilation heuristics do not account for value profile fullness

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

When tiered compilation decides to optimize a code block, some value profiles might not contain sufficient information to make wise optimization decisions.  Tiered compilation does not take this into account.  One way that tiered compilation could take this into account is to postpone optimization a bit if some value profiles are unusually empty.  It can also work around problems with value profiles containing only a few samples due to resonance in the bucketCounterRegister by converting the buckets into a prediction and then clearing them, so that even with resonance happening, after a few iterations of optimization postponement the prediction will contain everything we need to know.

[Filip Pizlo]

Created attachment 107395 [details]
the patch

[WebKit Review Bot]

Attachment 107395 [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/ValueProfile.h:278:  The parameter name "classInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/ValueProfile.h:278:  The parameter name "statistics" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/ValueProfile.h:281:  The parameter name "statistics" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 18 files


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

[Filip Pizlo]

Created attachment 107398 [details]
the patch - fix style

[Oliver Hunt]

Comment on attachment 107395 [details]
the patch

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

Basically okay, but it needs an actual changelog and can you convert the magic numbers into named constants?

Given you're adding a new cpp file you'll need to add it to GNUMakefile, JavaScriptCore.pro and CMakeLists.txt

> Source/JavaScriptCore/ChangeLog:7
> +

You do need an actual changelog entry you know :D

> Source/JavaScriptCore/jit/JIT.cpp:102
> -    Jump skipOptimize = branchAdd32(Signed, TrustedImm32(kind == LoopOptimizationCheck ? 1 : 30), AbsoluteAddress(&m_codeBlock->m_executeCounter));
> +    Jump skipOptimize = branchAdd32(Signed, TrustedImm32(kind == LoopOptimizationCheck ? 1 : 15), AbsoluteAddress(m_codeBlock->addressOfExecuteCounter()));

Ahhh magic numbers!!! L-( I have no idea what this number means -- can we hoist it into a named constant?

[Filip Pizlo]

Created attachment 107399 [details]
the patch

[Oliver Hunt]

Comment on attachment 107399 [details]
the patch

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

Correct the printf bit before landing.

Would be nice to avoid unnecessary renames (buckets->m_buckets, etc) in future, as they add noise to the patch.

> Source/JavaScriptCore/jit/JITStubs.cpp:1840
> +        if (error)
> +            fprintf(stderr, "WARNING: optimized compilation from loop failed.\n");

Should be in a VERBOSE_OSR block?

Future patch might want to define a OSR_LOG() macro which might be nicer in the long run

[Early Warning System Bot]

Comment on attachment 107399 [details]
the patch

Attachment 107399 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9661650

[Gyuyoung Kim]

Comment on attachment 107399 [details]
the patch

Attachment 107399 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9659685

[Filip Pizlo]

Landed in r95134.