Bug 68116 - Tiered compilation heuristics do not account for value profile fullness
Summary: Tiered compilation heuristics do not account for value profile fullness
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-14 14:12 PDT by Filip Pizlo
Modified: 2011-09-14 16:00 PDT (History)
4 users (show)

See Also:


Attachments
the patch (40.86 KB, patch)
2011-09-14 14:44 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch - fix style (40.83 KB, patch)
2011-09-14 14:50 PDT, Filip Pizlo
oliver: review-
Details | Formatted Diff | Diff
the patch (41.71 KB, patch)
2011-09-14 15:00 PDT, Filip Pizlo
oliver: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-09-14 14:12:43 PDT
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.
Comment 1 Filip Pizlo 2011-09-14 14:44:31 PDT
Created attachment 107395 [details]
the patch
Comment 2 WebKit Review Bot 2011-09-14 14:47:19 PDT
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.
Comment 3 Filip Pizlo 2011-09-14 14:50:48 PDT
Created attachment 107398 [details]
the patch - fix style
Comment 4 Oliver Hunt 2011-09-14 14:53:17 PDT
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?
Comment 5 Filip Pizlo 2011-09-14 15:00:54 PDT
Created attachment 107399 [details]
the patch
Comment 6 Oliver Hunt 2011-09-14 15:07:34 PDT
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
Comment 7 Early Warning System Bot 2011-09-14 15:09:17 PDT
Comment on attachment 107399 [details]
the patch

Attachment 107399 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9661650
Comment 8 Gyuyoung Kim 2011-09-14 15:13:49 PDT
Comment on attachment 107399 [details]
the patch

Attachment 107399 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9659685
Comment 9 Filip Pizlo 2011-09-14 16:00:36 PDT
Landed in r95134.