Bug 22482 - REGRESSION (r37991): Occasionally see "Scene rendered incorrectly" message when running the V8 Raytrace benchmark
Summary: REGRESSION (r37991): Occasionally see "Scene rendered incorrectly" message wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Oliver Hunt
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-11-25 01:31 PST by Cameron Zwarich (cpst)
Modified: 2008-12-02 13:52 PST (History)
3 users (show)

See Also:


Attachments
in-browser test case (222 bytes, text/html)
2008-11-25 04:49 PST, Alexey Proskuryakov
no flags Details
Patch to fix problem (15.92 KB, patch)
2008-11-30 08:45 PST, Cameron Zwarich (cpst)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-11-25 01:31:25 PST
When running the V8 Raytrace benchmark, many of us have seen the "Scene rendered incorrectly" message. It doesn't always occur, so I have always suspected that it is a GC issue. If I hack up the V8 test harness, it always occurs with the JIT and COLLECT_ON_EVERY_ALLOCATION. It does not occur with the bytecode interpreter, even with COLLECT_ON_EVERY_ALLOCATION.

I will try to reduce and fix this. I suspect that it is caused by some bad interaction between inline caching and GC.
Comment 1 Cameron Zwarich (cpst) 2008-11-25 03:20:49 PST
Here is a reduction for the jsc shell, only with COLLECT_ON_EVERY_ALLOCATION:

var o = {
    x: 0.2,

    normalize: function() {
        return this.x / 5.0039984012787215;
    }
};

print(-o.normalize());

This prints 0, but it should print something nonzero. I believe that it happens because a number cell is being collected. I can't trigger it by putting explicit gc() calls anywhere.
Comment 2 Cameron Zwarich (cpst) 2008-11-25 04:05:08 PST
The problem seems to be caused by the negation, as it goes away if I remove it. The GC caused by the call to allocateNumber() emitted by

                if (!resultType.isReusableNumber())
                    emitAllocateNumber(m_globalData, i);

must not be marking the operand to op_negate.
Comment 3 Cameron Zwarich (cpst) 2008-11-25 04:10:18 PST
Here is a simpler reduction:

function f()
{
    return 0.1;
}

print(-f());
Comment 4 Alexey Proskuryakov 2008-11-25 04:49:24 PST
Created attachment 25478 [details]
in-browser test case

Same as the above, but doesn't need COLLECT_ON_EVERY_ALLOCATION.
Comment 5 Cameron Zwarich (cpst) 2008-11-25 14:09:00 PST
This is likely caused by r37991:

http://trac.webkit.org/changeset/37991

Oliver says that he probably has a fix.
Comment 6 Cameron Zwarich (cpst) 2008-11-25 14:43:05 PST
I have verified that this is caused by r37991.
Comment 7 Cameron Zwarich (cpst) 2008-11-25 14:48:59 PST
Oliver indeed has a fix for this, but it is part of other work. The problem is that xmm0 is not callee-save, so eventually some function (like memcpy()) will get called and smash its value. I am assigning this to Oliver.
Comment 8 Cameron Zwarich (cpst) 2008-11-30 08:45:06 PST
Created attachment 25614 [details]
Patch to fix problem

This patch simply reverts r37991. I can't accurately test its performance. It seems to be a regression, but my loaner machine has insane variance.
Comment 9 Geoffrey Garen 2008-12-02 13:52:32 PST
Comment on attachment 25614 [details]
Patch to fix problem

r=me

Performance seems OK.
Comment 10 Geoffrey Garen 2008-12-02 13:52:40 PST
Committed revision 38917.