Bug 131906 – Take block execution count estimates into account when voting double

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 229754 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:100:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:114:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:119:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:138:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:148:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.h:202:  The parameter name "doubleFormatState" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 14 files


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

[Filip Pizlo]

(In reply to comment #2)
> Attachment 229754 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:100:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:114:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:119:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:138:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp:148:  Multi line control clauses should use braces.  [whitespace/braces] [4]

No.

> ERROR: Source/JavaScriptCore/dfg/DFGVariableAccessData.h:202:  The parameter name "doubleFormatState" adds no information, so it should be removed.  [readability/parameter_name] [5]

Fixed.

> Total errors found: 7 in 14 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 229754 [details]
the patch

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

r=me

> Source/JavaScriptCore/dfg/DFGNodeFlags.h:63
> +#define NodeBytecodeReallyWantsInt       0x2000 // The result of this computation is known to be used in a context that strongly prefers integer values, to the point that we should avoid using doubles if at all possible.

I'm not a big fan of this name. "Really" doesn't do much to explain the difference from UsesAsInt.

How about "NodeBytecodeUsesAsArrayIndex"?

> Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:86
> -        BasicBlock* newRoot = insertionSet.insert(0, PNaN);
> +        BasicBlock* newRoot = insertionSet.insert(0, 1);

Why 1 instead of NaN? I thought one goal of this patch was to keep the entry point as NaN, and require clients to be NaN-friendly.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 229754 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229754&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGNodeFlags.h:63
> > +#define NodeBytecodeReallyWantsInt       0x2000 // The result of this computation is known to be used in a context that strongly prefers integer values, to the point that we should avoid using doubles if at all possible.
> 
> I'm not a big fan of this name. "Really" doesn't do much to explain the difference from UsesAsInt.
> 
> How about "NodeBytecodeUsesAsArrayIndex"?

Yeah, that's better.  I'll change.

> 
> > Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:86
> > -        BasicBlock* newRoot = insertionSet.insert(0, PNaN);
> > +        BasicBlock* newRoot = insertionSet.insert(0, 1);
> 
> Why 1 instead of NaN? I thought one goal of this patch was to keep the entry point as NaN, and require clients to be NaN-friendly.

:-/  One of the goals of this patch is indeed to ensure that if you use executionCount, you should have a sensible default if it's NaN.  One of the places where it *should* be NaN is for injected loop pre-headers and broken critical edges, because: (1) these blocks are synthetic so we don't know for sure how frequently they execute and we wouldn't know even if we had bytecode-level execution count profiling, and (2) these blocks contain no real code so it wouldn't be worth it to put effort towards trying to measure or recover an execution count for them.

In this code, the OSR entrypoint phase is creating a different kind of block, that is neither a broken critical edge nor a loop pre-header.  It's the entrypoint itself.  And we know for sure that under the definition of "executionCount" the entrypoint executes once, since "1" means "1 execution per invocation" - so the entrypoint is definitely something that executes once.

On the other hand, it's true that using NaN here would be fine because it would signal to the clients of executionCount to figure it out for themselves and currently the clients of this data use NaN to mean "it didn't execute a lot".  Using 0 would be fine, too.  Basically, using anything that means "it didn't execute a lot" would be fine.  (Strictly speaking, using *any* value would be "correct" since it's just profiling, but what we want is just a value that indicates that it didn't run a lot and 1 will do fine.)  However, since we can be sure that under the definition of executionCount, 1 most accurately describes the execution count of this block, we might as well just use that.

I'll try to sum this up somehow in a comment before I land.

[Geoffrey Garen]

> However, since we can be sure that under the definition of executionCount, 1 most accurately describes the execution count of this block, we might as well just use that.

Makes sense.

I think the thing that could have helped me understand this best would have been:

Your ChangeLog comment, "Realize that OSR entrypoint creation creates blocks that have NaN execution count". I assumed, wrongly, that the entrypoint itself would be NaN, but actually you meant a loop pre-header created by the entrypoint.

[Filip Pizlo]

Landed in https://trac.webkit.org/r167600