Bug 147165 – Occasional failure in v8-v6/v8-raytrace.js.ftl-eager

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

[Filip Pizlo]

Earliest revision that I see it in is r187107.

[Filip Pizlo]

[pizlo@dethklok OpenSource] DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ until_fail.rb WebKitBuild/Release/jsc --useFTLJIT\=false --enableFunctionDotArguments\=true --useFTLJIT\=true --ftlCrashesIfCantInitializeLLVM\=true --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js
Doing run #1...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #2...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #3...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #4...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #5...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #6...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #7...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #8...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #9...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #10...
Running ["WebKitBuild/Release/jsc", "--useFTLJIT=false", "--enableFunctionDotArguments=true", "--useFTLJIT=true", "--ftlCrashesIfCantInitializeLLVM=true", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Exception: TypeError: this.initialize.apply is not a function. (In 'this.initialize.apply(this, arguments)', 'this.initialize.apply' is undefined)
PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:31:28
testIntersection@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:677:55
rayTrace@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:768:51
getPixelColor@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:669:38
renderScene@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:656:43
renderScene@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:898:26
global code@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:902:14

[Filip Pizlo]

This worked for me too:

DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ until_fail.rb WebKitBuild/Release/jsc "--thresholdForJITAfterWarmUp=10" "--thresholdForJITSoon=10" "--thresholdForOptimizeAfterWarmUp=20" "--thresholdForOptimizeAfterLongWarmUp=20" "--thresholdForOptimizeSoon=20" "--thresholdForFTLOptimizeAfterWarmUp=20" "--thresholdForFTLOptimizeSoon=20" "--maximumEvalCacheableSourceLength=150000" --enableObjectAllocationSinking=false PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js

I last tested this on r188482 on Yosemite.

[Filip Pizlo]

(In reply to comment #2)
> This worked for me too:
> 
> DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ until_fail.rb
> WebKitBuild/Release/jsc "--thresholdForJITAfterWarmUp=10"
> "--thresholdForJITSoon=10" "--thresholdForOptimizeAfterWarmUp=20"
> "--thresholdForOptimizeAfterLongWarmUp=20" "--thresholdForOptimizeSoon=20"
> "--thresholdForFTLOptimizeAfterWarmUp=20" "--thresholdForFTLOptimizeSoon=20"
> "--maximumEvalCacheableSourceLength=150000"
> --enableObjectAllocationSinking=false
> PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js
> 
> I last tested this on r188482 on Yosemite.

I lied.  The command line I used was:

[pizlo@shakezilla OpenSource] DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ until_fail.rb WebKitBuild/Release/jsc "--thresholdForJITAfterWarmUp=10" "--thresholdForJITSoon=10" "--thresholdForOptimizeAfterWarmUp=20" "--thresholdForOptimizeAfterLongWarmUp=20" "--thresholdForOptimizeSoon=20" "--thresholdForFTLOptimizeAfterWarmUp=20" "--thresholdForFTLOptimizeSoon=20" "--maximumEvalCacheableSourceLength=150000" PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js
Doing run #1...
Running ["WebKitBuild/Release/jsc", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #2...
Running ["WebKitBuild/Release/jsc", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Doing run #3...
Running ["WebKitBuild/Release/jsc", "--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20", "--maximumEvalCacheableSourceLength=150000", "PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js"]
Exception: TypeError: this.initialize.apply is not a function. (In 'this.initialize.apply(this, arguments)', 'this.initialize.apply' is undefined)
PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:31:28
initialize@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:527:46
PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:31:28
intersect@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:474:55
testIntersection@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:684:43
rayTrace@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:768:51
rayTrace@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:745:45
getPixelColor@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:669:38
renderScene@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:656:43
renderScene@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:898:26
global code@PerformanceTests/SunSpider/tests/v8-v6/v8-raytrace.js:902:14
/Users/pizlo/bin/until_fail.rb:8:in `block in <main>': fail after 3 runs: #<Process::Status: pid 2950 exit 3> (RuntimeError)
	from /Users/pizlo/bin/until_fail.rb:4:in `loop'
	from /Users/pizlo/bin/until_fail.rb:4:in `<main>'

[Radar WebKit Bug Importer]

<rdar://problem/22293750>

[Basile Clement]

Created attachment 259059 [details]
Patch

[Basile Clement]

I locally changed any reference to "sinking MultiGetByOffset" into "lowering MultiGetByOffset" in the ChangeLog.

[Saam Barati]

Comment on attachment 259059 [details]
Patch

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

r=me.

> Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js:2
> +Foo.prototype.f = 42;

should all "f"s be 42?

> Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js:24
> +        throw new Error("Result should be 12 but was " + result);

"result should be 12" => "result should be 42"

[Basile Clement]

(In reply to comment #7)
> Comment on attachment 259059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259059&action=review
> 
> r=me.
> 
> > Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js:2
> > +Foo.prototype.f = 42;
> 
> should all "f"s be 42?

Yes! In current trunk, the output of this test is:

> Exception: Error: Result should be 12 but was 1927

I realize this test probably looks super confusing, I'll add a reference before landing stating that it is a regression test for this bug so that people can find the following explanation.

> function get(o, p) {
>     if (p)
>         return o.f;
>     return 42;
> }
>
> for (var i = 0; i < 100000; ++i) {
>     get({ f: 42 }, i % 2);
>     get({ o: 10, f: 42 }, i % 2);
> }

This forces the "o.f" access to be profiled as always using either a {f:0} or {o:0,f:1} structure.

> function foo() {
>     var o = new Foo();
>     return get(o, isFinalTier());
> }
> noInline(foo);
>
> for (var i = 0; i < 1000000; ++i) {
>     var result = foo();
>     if (result !== 42)
>         throw new Error("Result should be 12 but was " + result);
> }

Due to the forced profiling, and since before getting to the FTL the second parameter to get() is always false (i.e. the "o.f" access never gets profiled with a Foo object), the graph for foo looks like this (isFinalTier() being an intrinsic, it is constant-folded to true when compiling for the FTL):

    <1> NewObject([prototype={f:0}])
    <2> MultiGetByOffset(@1, f, [{f:0},{o:0,f:1}])
    <3> Return(@2)

Here, we will fail the structure check at @2, and OSR exit.
With allocation sinking enabled however, we currently make the (wrong) assumption that a MultiGetByOffset can't ever exit, and that (much like a GetByOffset) it must have been protected by a previous CheckStructure. This is obviously wrong, and will lead to the graph being transformed into something like this (1927 is a dummy default value that should never be read and is only present so that we can build valid Phi graphs):

    <4> JSConstant(1927)
    <1> PhantomNewObject()
    <5> JSConstant([prototype={f:0}])
    <6> PutHint(@1, StructurePLoc, @5)
    <7> PutHint(@2, "f", @0)
    <2> Check()
    <3> Return(@4)

And now we're dead, because instead of exiting @2 is a no-op and we return an out-of-thin-air 1927.

With this patch, we will instead generate the following (correct) graph:


    <4> JSConstant(1927)
    <1> PhantomNewObject()
    <5> JSConstant([prototype={f:0}])
    <6> PutHint(@1, StructurePLoc, @5)
    <7> PutHint(@2, "f", @0)
    <2> CheckStructureImmediate(@5, [])
    <3> Return(@4)

> 
> > Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js:24
> > +        throw new Error("Result should be 12 but was " + result);
> 
> "result should be 12" => "result should be 42"

Good catch, I'll change that.

[Basile Clement]

Created attachment 259080 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 259080 [details]
Patch for landing

Clearing flags on attachment: 259080

Committed r188507: <http://trac.webkit.org/changeset/188507>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 259059 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=259059&action=review
> > 
> > r=me.
> > 
> > > Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js:2
> > > +Foo.prototype.f = 42;
> > 
> > should all "f"s be 42?
> 
> Yes! In current trunk, the output of this test is:
> 
> > Exception: Error: Result should be 12 but was 1927
> 
> I realize this test probably looks super confusing, I'll add a reference
> before landing stating that it is a regression test for this bug so that
> people can find the following explanation.
> 
> > function get(o, p) {
> >     if (p)
> >         return o.f;
> >     return 42;
> > }
> >
> > for (var i = 0; i < 100000; ++i) {
> >     get({ f: 42 }, i % 2);
> >     get({ o: 10, f: 42 }, i % 2);
> > }
> 
> This forces the "o.f" access to be profiled as always using either a {f:0}
> or {o:0,f:1} structure.
> 
> > function foo() {
> >     var o = new Foo();
> >     return get(o, isFinalTier());
> > }
> > noInline(foo);
> >
> > for (var i = 0; i < 1000000; ++i) {
> >     var result = foo();
> >     if (result !== 42)
> >         throw new Error("Result should be 12 but was " + result);
> > }
> 
> Due to the forced profiling, and since before getting to the FTL the second
> parameter to get() is always false (i.e. the "o.f" access never gets
> profiled with a Foo object), the graph for foo looks like this
> (isFinalTier() being an intrinsic, it is constant-folded to true when
> compiling for the FTL):
> 
>     <1> NewObject([prototype={f:0}])
>     <2> MultiGetByOffset(@1, f, [{f:0},{o:0,f:1}])
>     <3> Return(@2)
> 
> Here, we will fail the structure check at @2, and OSR exit.
> With allocation sinking enabled however, we currently make the (wrong)
> assumption that a MultiGetByOffset can't ever exit, and that (much like a
> GetByOffset) it must have been protected by a previous CheckStructure. This
> is obviously wrong, and will lead to the graph being transformed into
> something like this (1927 is a dummy default value that should never be read
> and is only present so that we can build valid Phi graphs):
> 
>     <4> JSConstant(1927)
>     <1> PhantomNewObject()
>     <5> JSConstant([prototype={f:0}])
>     <6> PutHint(@1, StructurePLoc, @5)
>     <7> PutHint(@2, "f", @0)
>     <2> Check()
>     <3> Return(@4)
> 
> And now we're dead, because instead of exiting @2 is a no-op and we return
> an out-of-thin-air 1927.
> 
> With this patch, we will instead generate the following (correct) graph:
> 
> 
>     <4> JSConstant(1927)
>     <1> PhantomNewObject()
>     <5> JSConstant([prototype={f:0}])
>     <6> PutHint(@1, StructurePLoc, @5)
>     <7> PutHint(@2, "f", @0)
>     <2> CheckStructureImmediate(@5, [])
>     <3> Return(@4)

Thanks for clarifying. These examples helped me better understand.