Bug 186916 – We can't remove code after ForceOSRExit until after FixupPhase

• Component: JavaScriptCore
• Status: RESOLVED FIXED

We encounter issue when division calculation result assigned to variable results in variable having 0 value when it shouln't.

I've attached test html which demonstrates this issue (probably could be reduced to much smaller test case).
Steps to reproduce:
1. load page
2. in top left corner there will be canvas and you need to draw something using touch
3. click button
It will read image from canvas, calculate number of filled dots and divide by number of all dots to receive fill factor.
This operation performed 100 times and log attempts, when fill factor resulted in 0. For us it fails 5-6 first times. Note that divisor and divider populated properly and it is only result not saved properly to variable.
To repeat test you need to reload test page.

Environment details:
we can reproduce it on iOS 11.3-11.4 iPhone8/iPad
we cannot reproduce it on iOS 11.0 iPhone8

Created attachment 343322 [details]
test case

[Alexey Proskuryakov]

Does this reproduce for you with iOS 12 beta?

[Radar WebKit Bug Importer]

<rdar://problem/41396612>

Sorry, I'm afraid we don't have such device available, as we mostly use 3rd-party test environment providers and they don't have this version available.

Just tested it on iOS 12/iPhone and it still fails to produce proper results first ~10 iterations

[Saam Barati]

This happens in all tiers. If I only use the LLInt, it still happens. If I just use baseline, it still happens. If I use baselineJIT+, it still happens. If I turn off GC, this still happens.

Doesn't feel like a JSC bug to me, but perhaps it is. I wonder if it could be something w/ how canvas is filling in the buffers.

[Saam Barati]

(In reply to Saam Barati from comment #6)
> This happens in all tiers. If I only use the LLInt, it still happens. If I
> just use baseline, it still happens. If I use baselineJIT+, it still
> happens. If I turn off GC, this still happens.
> 
> Doesn't feel like a JSC bug to me, but perhaps it is. I wonder if it could
> be something w/ how canvas is filling in the buffers.

Weird, I am now reading the code. And it's literally the same calculation in two different places... Doesn't look like a canvas bug.

[Saam Barati]

(In reply to Saam Barati from comment #6)
> This happens in all tiers. If I only use the LLInt, it still happens. If I
> just use baseline, it still happens. If I use baselineJIT+, it still
> happens. If I turn off GC, this still happens.
> 
> Doesn't feel like a JSC bug to me, but perhaps it is. I wonder if it could
> be something w/ how canvas is filling in the buffers.

It looks like I was incorrectly setting environment variables. So my test wasn't valid.

[Saam Barati]

This is a DFG bug. It happens if I turn off the FTL. It doesn't happen if I turn off the DFG

[Saam Barati]

Created attachment 359650 [details]
Mac test case

[Saam Barati]

I know what's going on. We end up with IR like:

a: ArithDiv
b: MovHint(@a)
c: ForceOSRExit

However, for @a, we think we don't need to check that the remainder is non-zero. I need to figure out why backwards propagation is failing to say we care about this value.

[Saam Barati]

MovHint does nothing in backwards propagation. I wonder why... That feels wrong.

[Saam Barati]

(In reply to Saam Barati from comment #11)
> I know what's going on. We end up with IR like:
> 
> a: ArithDiv
> b: MovHint(@a)
> c: ForceOSRExit
> 
> However, for @a, we think we don't need to check that the remainder is
> non-zero. I need to figure out why backwards propagation is failing to say
> we care about this value.

This code is what we generate in the "var fillRatio = filled / pixels;"

[Saam Barati]

Reduced test for JSC shell:

```
function foo(v, a, b) {
    if (v) {
        let r = a / b;
        OSRExit();
        return r;
    }
}
noInline(foo);

for (let i = 0; i < 10000; ++i) {
    let r = foo(true, 4, 4);
    if (r !== 1)
        throw new Error("Bad!");
}
if (foo(true, 1, 4) !== 0.25)
    throw new Error("Bad!");
```

[Saam Barati]

Created attachment 359658 [details]
patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


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

[Saam Barati]

Created attachment 359659 [details]
patch

[Saam Barati]

Created attachment 359660 [details]
patch

[Yusuke Suzuki]

Comment on attachment 359660 [details]
patch

r=me

[EWS Watchlist]

Comment on attachment 359660 [details]
patch

Attachment 359660 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10823730

New failing tests:
stress/arith-abs-to-arith-negate-range-optimizaton.js.no-cjit-validate-phases
stress/arith-abs-to-arith-negate-range-optimizaton.js.no-ftl
stress/arith-abs-to-arith-negate-range-optimizaton.js.default
stress/arith-abs-to-arith-negate-range-optimizaton.js.ftl-no-cjit-no-inline-validate
stress/arith-abs-to-arith-negate-range-optimizaton.js.no-cjit-collect-continuously
stress/arith-abs-to-arith-negate-range-optimizaton.js.ftl-no-cjit-validate-sampling-profiler
stress/arith-abs-to-arith-negate-range-optimizaton.js.ftl-no-cjit-b3o1
stress/arith-abs-to-arith-negate-range-optimizaton.js.no-llint
apiTests

[Saam Barati]

Created attachment 359664 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 359664 [details]
patch for landing

Clearing flags on attachment: 359664

Committed r240223: <https://trac.webkit.org/changeset/240223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

This is a 1% regression on JetStream 2. It regresses float-mm, crypto, octane-zlib by quite a lot. We need to think about how to do this without affecting performance. (Thought AFAICT, code is just wrong without this patch, so it may take some creativity to teach OSR how to materialize the bits it needs)

[Saam Barati]

rolled out in:
https://trac.webkit.org/changeset/240269/webkit

[Saam Barati]

I'm beginning to think that this analysis is only sound over bytecode. Alternatively, we could teach the DFG to recover the original value and only perform this optimization if it's able to recover the value. Like OSR exit can easily replay an `add` without doing side effects.

The reason I think this regressed is that I added code into BytecodeParser that made us emit Unreachable after ForceOSRExit. Before that, we'd be able to better "analyze" what the bytecode was doing because we saw past these ForceOSRExit nodes. However, relying on that is super hacky. We should probably solve this in a general way.

[Filip Pizlo]

(In reply to Saam Barati from comment #26)
> I'm beginning to think that this analysis is only sound over bytecode.
> Alternatively, we could teach the DFG to recover the original value and only
> perform this optimization if it's able to recover the value. Like OSR exit
> can easily replay an `add` without doing side effects.

Right now it's sound because the ByteCodeParser creates DFG IR that has a complete view of exits, either because we emit real Nodes for all uses in bytecode, or we emit Phantoms to simulate those uses.

Based on this, our current approach of backwards-analyzing DFG IR is sound, except if you do the Unreachable code killing thing in the ByteCodeParser.  It's generally not OK to do optimizations in ByteCodeParser.  We can do some, but we have to be super careful.

> 
> The reason I think this regressed is that I added code into BytecodeParser
> that made us emit Unreachable after ForceOSRExit. Before that, we'd be able
> to better "analyze" what the bytecode was doing because we saw past these
> ForceOSRExit nodes. However, relying on that is super hacky. We should
> probably solve this in a general way.

Philosophically, I support eventually doing the BackwardsPropagationPhase over bytecode.  But we should be careful about making that move.  I think that there are perf benefits to doing the analysis over DFG IR, since it means that the results of the analysis are immediately expressed inside the DFG IR data structures and not somewhere else.

So, it may be that the way we do it right now is the best.

[Saam Barati]

(In reply to Filip Pizlo from comment #27)
> (In reply to Saam Barati from comment #26)
> > I'm beginning to think that this analysis is only sound over bytecode.
> > Alternatively, we could teach the DFG to recover the original value and only
> > perform this optimization if it's able to recover the value. Like OSR exit
> > can easily replay an `add` without doing side effects.
> 
> Right now it's sound because the ByteCodeParser creates DFG IR that has a
> complete view of exits, either because we emit real Nodes for all uses in
> bytecode, or we emit Phantoms to simulate those uses.
> 
> Based on this, our current approach of backwards-analyzing DFG IR is sound,
> except if you do the Unreachable code killing thing in the ByteCodeParser. 
> It's generally not OK to do optimizations in ByteCodeParser.  We can do
> some, but we have to be super careful.

Maybe we should make this optimization just emit Phantoms. I'll first run some perf numbers to see if we can just remove this optimization.

> 
> > 
> > The reason I think this regressed is that I added code into BytecodeParser
> > that made us emit Unreachable after ForceOSRExit. Before that, we'd be able
> > to better "analyze" what the bytecode was doing because we saw past these
> > ForceOSRExit nodes. However, relying on that is super hacky. We should
> > probably solve this in a general way.
> 
> Philosophically, I support eventually doing the BackwardsPropagationPhase
> over bytecode.  But we should be careful about making that move.  I think
> that there are perf benefits to doing the analysis over DFG IR, since it
> means that the results of the analysis are immediately expressed inside the
> DFG IR data structures and not somewhere else.
> 
> So, it may be that the way we do it right now is the best.

Right. If we want to keep this optimization, I see two potential paths forward:
1. Make it use Phantom and keep it in bytecode parser.
2. Make it a helper phase that we run after fixup.

[Filip Pizlo]

(In reply to Saam Barati from comment #28)
> (In reply to Filip Pizlo from comment #27)
> > (In reply to Saam Barati from comment #26)
> > > I'm beginning to think that this analysis is only sound over bytecode.
> > > Alternatively, we could teach the DFG to recover the original value and only
> > > perform this optimization if it's able to recover the value. Like OSR exit
> > > can easily replay an `add` without doing side effects.
> > 
> > Right now it's sound because the ByteCodeParser creates DFG IR that has a
> > complete view of exits, either because we emit real Nodes for all uses in
> > bytecode, or we emit Phantoms to simulate those uses.
> > 
> > Based on this, our current approach of backwards-analyzing DFG IR is sound,
> > except if you do the Unreachable code killing thing in the ByteCodeParser. 
> > It's generally not OK to do optimizations in ByteCodeParser.  We can do
> > some, but we have to be super careful.
> 
> Maybe we should make this optimization just emit Phantoms. I'll first run
> some perf numbers to see if we can just remove this optimization.
> 
> > 
> > > 
> > > The reason I think this regressed is that I added code into BytecodeParser
> > > that made us emit Unreachable after ForceOSRExit. Before that, we'd be able
> > > to better "analyze" what the bytecode was doing because we saw past these
> > > ForceOSRExit nodes. However, relying on that is super hacky. We should
> > > probably solve this in a general way.
> > 
> > Philosophically, I support eventually doing the BackwardsPropagationPhase
> > over bytecode.  But we should be careful about making that move.  I think
> > that there are perf benefits to doing the analysis over DFG IR, since it
> > means that the results of the analysis are immediately expressed inside the
> > DFG IR data structures and not somewhere else.
> > 
> > So, it may be that the way we do it right now is the best.
> 
> Right. If we want to keep this optimization, I see two potential paths
> forward:
> 1. Make it use Phantom and keep it in bytecode parser.
> 2. Make it a helper phase that we run after fixup.

The phantom approach is going to be faster. So we should do that. 

Ugh. I don’t like bytecode parser optimizations at all but perf is perf and the third tier is not about being pretty.

[Saam Barati]

Created attachment 364048 [details]
WIP

I think this should work. I'm first going to run an A/B test to see if we can just completely get rid of this code instead of making this change. If the A/B tests say we need this code, I'll prepare a full patch for review.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7329:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 2 files


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

[EWS Watchlist]

Comment on attachment 364048 [details]
WIP

Attachment 364048 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11430768

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 364066 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Saam Barati]

Mac data so far:
Up to 0.5% worse on Speedometer2, but not on every device.
Neutral on JS2 on iMac. I'm kicking off some tasks to run it on more Mac HW models.


I don't yet have iOS data as some tasks I kicked off did not succeed. Hopefully I will have that data soon.

[Saam Barati]

Created attachment 364562 [details]
patch

[Yusuke Suzuki]

Comment on attachment 364562 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:16
> +        There was an optimization on the bytecode parser I added that converted blocks
> +        with ForceOSRExit in them to remove all IR after the ForceOSRExit. However,
> +        this is incorrect because it breaks backwards propagation. For example, it
> +        could incorrectly lead us to think it's safe to not check for overflow in
> +        an Add because such Add has no non-int uses. Backwards propagation relies on
> +        having a view over bytecode uses, and this optimization broke that. This patch
> +        rolls out that optimization, as initial perf data shows it may no longer be
> +        needed.

You can mention to r232742 here. This patch is logically the revert of r232742.

[Saam Barati]

Comment on attachment 364562 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:16
>> +        needed.
> 
> You can mention to r232742 here. This patch is logically the revert of r232742.

Good point. Will do.

[Saam Barati]

Created attachment 364757 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 364757 [details]
patch for landing

Clearing flags on attachment: 364757

Committed r242989: <https://trac.webkit.org/changeset/242989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.