Closed
Bug 620860
Opened 14 years ago
Closed 13 years ago
Assertion failure: frame entry 172 wasn't freed
Categories
(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P1)
Tamarin Graveyard
Baseline JIT (CodegenLIR)
Tracking
(Not tracked)
RESOLVED
FIXED
Q4 11 - Anza
People
(Reporter: edwsmith, Assigned: stejohns)
References
Details
Attachments
(4 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
edwsmith
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
wmaddox
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
Found in:
swfcontents/y/Q/q/e/Y/YqQeYqtmpL2mtsnSJo2efZRkdzTX+Z3E2mCgrtuZgUI=
I don't know what the problem is yet, but I want to get the bug
entered anyway so we don't lose it.
Other than being some random assert, a few interesting things appear
in the LIR listing:
1. the function name is mx.utils::UIDUtil$/isUID(). So this is our
code, not some random thing off the web.
2. the LIR listing has calls to multiple variants of stringCharCodeAt()
with the same args:
81: String_charCodeAtFU1 = calld.none #String_charCodeAtFU ( ldq10 ldi2 )
131: String_charCodeAtIU1 = calli.none #String_charCodeAtIU ( ldq10 ldi2 )
There looks like quite a bit of branchy control flow, too.
Reporter | ||
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Future
Reporter | ||
Updated•14 years ago
|
Target Milestone: Future → flash9.0.x-Spicy
Reporter | ||
Updated•14 years ago
|
Target Milestone: flash9.0.x-Spicy → flash10.2.x-Spicy
Reporter | ||
Updated•14 years ago
|
Attachment #499221 -
Attachment mime type: application/octet-stream → text/plain
Reporter | ||
Updated•14 years ago
|
Priority: -- → P1
Reporter | ||
Comment 1•14 years ago
|
||
As a data point, x86 fails with a different assertion:
Assertion failure: (_allocator.active[REGNUM(FST0)] && _fpuStkDepth == -1) || (!_allocator.active[REGNUM(FST0)] && _fpuStkDepth == 0) (/Users/edwin/vm/jitrefac/nanojit/Assembler.cpp:371)
Reporter | ||
Comment 2•14 years ago
|
||
this bug is a possible duplicate of bug 614510; not marking it yet until it is investigated.
Comment 3•14 years ago
|
||
I can't find file swfcontents/y/Q/q/e/Y/YqQeYqtmpL2mtsnSJo2efZRkdzTX+Z3E2mCgrtuZgUI=.
Can you run with ' -Dverbose=jit,verify,raw,regs ' before and after applying patches from bug 607816.
You should be able to see something similar to what's described in comment 3 in bug 614510 if this is indeed the same problem.
And btw the issues manifests as a 'frame entry wasn't freed' assertion, not an fpu assertion:
Eg. frame entry xxx wasn't freed : _entries[i] == __null
Moving from Spicy to Wasabi for investigation.
Flags: flashplayer-qrb+
Flags: flashplayer-bug+
Target Milestone: flash10.2.x-Spicy → flash10.x-Wasabi
Updated•14 years ago
|
Assignee: nobody → rreitmai
Comment 5•14 years ago
|
||
Most likely dup of bug 609028
Updated•14 years ago
|
Target Milestone: Q2 11 - Wasabi → Q3 11 - Serrano
Comment 7•13 years ago
|
||
This should already be fixed, it just need to be validated against the test case outlined in the bug desc.
Assignee | ||
Comment 8•13 years ago
|
||
Here's a simple script that will demo the problem: rebuild it and run using a Debug Debugger build. If you run 'normally' (jit or interp) it works; if you run with -Dnodebugger, it asserts, and if continued after the assert, incorrectly prints "false" instead of "true".
Attachment #547586 -
Flags: feedback?
Assignee | ||
Comment 9•13 years ago
|
||
Comment on attachment 547586 [details] [diff] [review]
Bad source
Note also that a Release build will (incorrectly) print false unless you force -Dinterp.
Attachment #547586 -
Flags: feedback?(wmaddox)
Attachment #547586 -
Flags: feedback?(edwsmith)
Attachment #547586 -
Flags: feedback?
Assignee | ||
Comment 10•13 years ago
|
||
Even simpler testcase that demonstrates the problem. Note that if you change "var c:Number" into "var c:uint", the problem goes away.
Assignee | ||
Comment 11•13 years ago
|
||
Digging further, it appears that the specialization in optimizeIntCmpWithNumberCall() is questionable in this case (though it's not clear whether/why it's causing the misbehavior)... we have already called charCodeAt(), and since the result is stored into an explicit Number type, we specialize to the "FU" variant returning float. However, in test vs literal '45', we note that it's an integer literal compared against charCodeAt result, and emit another call to the "IU" variant returning int. Yikes, certainly not a win to make redundant calls.
Assignee | ||
Comment 12•13 years ago
|
||
OK, so the problem is that the FU->IU specialization occurs in both branches, but in the 'else' branch, we reuse the same LIns... but that's only valid in the 'if' branch since that's the call site.
-- Note 1: it's a little disturbing that the LIns-recycling-code can emit stale stuff this way.
-- Note 2: even if it didn't recycle, we'd still have potentially suboptimal code since we'd be emitting two charCodeAt() calls where one would suffice.
Assignee | ||
Comment 13•13 years ago
|
||
Aha! The gotcha here is in specializeIntCall(); it attempts to avoid redundant calls by keeping a hashmap of specialized calls it's emitted, but this doesn't consider that they may have been emitted in code paths that are mutually exclusive...
Assignee | ||
Comment 14•13 years ago
|
||
Here's a simplistic fix that seems to address the issue: clear the specialized-function map when we insert a label. This is suboptimal, in that we may well be inserting redundant calls in some cases, but it should eliminate incorrect usage.
It's not clear to me if there's an existing structure in the JIT that allows us to do a smarter job of hoisting potentially redundant pure calls in this way -- is there?
Assignee: rreitmai → stejohns
Attachment #547786 -
Flags: superreview?(edwsmith)
Attachment #547786 -
Flags: review?(wmaddox)
Assignee | ||
Comment 15•13 years ago
|
||
Review ping?
Comment 16•13 years ago
|
||
(In reply to comment #15)
> Review ping?
This is a bit conservative, as you noted: If one specialization occurs above a control-flow diamond, and another identical one below, we'll duplicate the call even though the first dominates the second. Unfortunately, we don't have the machinery to do better than this, so I think the patch is a reasonable way to restore correctness.
Aside from this bug, the specializer has the weakness that we may have already been forced to emit an unspecialized call anyway by the time we come to a use that allows for a context-dependent specialization. At runtime, we may end of calling the both unspecialized version and a specialization, when it might be
cheaper just to coerce the result of the unspecialized call. This specializer
is really on shaky ground, and probably looked good on benchmarks only because the screw cases are rare in practice, or perhaps just in the few benchmarks measured.
I think it may be advisable to re-run the benchmarks upon which this optimization was originally justified, and make sure we're still seeing a net win.
Reporter | ||
Comment 17•13 years ago
|
||
Comment on attachment 547586 [details] [diff] [review]
Bad source
yep, pretty bad source! great test case.
Attachment #547586 -
Flags: feedback?(wmaddox) → feedback+
Reporter | ||
Updated•13 years ago
|
Attachment #547586 -
Flags: feedback?(edwsmith)
Reporter | ||
Comment 18•13 years ago
|
||
Comment on attachment 547786 [details] [diff] [review]
Patch
Patch looks find contingent on test cases and no serious regressions, as Bill mentioned.
Attachment #547786 -
Flags: superreview?(edwsmith) → superreview+
Assignee | ||
Comment 19•13 years ago
|
||
> I think it may be advisable to re-run the benchmarks upon which this
> optimization was originally justified, and make sure we're still seeing a
> net win.
Do we know which benchmarks those were? This needs to land in Serrano, which is scheduled for ZBB by end of week, so there's some urgency here.
Reporter | ||
Comment 20•13 years ago
|
||
the main ones have charCodeAt in their name (under test/acceptance/jsmicro and asmicro), but looking for new outliers would be good too.
Assignee | ||
Comment 21•13 years ago
|
||
(In reply to comment #20)
> the main ones have charCodeAt in their name (under test/acceptance/jsmicro
> and asmicro), but looking for new outliers would be good too.
Good call. I'll go ahead and run before-and-after benchmarks on those, at least.
Assignee | ||
Comment 22•13 years ago
|
||
before-and-after on mac-x86 and mac-x64 don't show any red flags. I'm going to land it if/when William gives the R+.
Comment 23•13 years ago
|
||
Comment on attachment 547786 [details] [diff] [review]
Patch
My concerns were only with respect to possible performance regression. From a correctness viewpoint, it looks sound. R+
Assignee | ||
Comment 24•13 years ago
|
||
pushed to TR: 6490:8de850f0d909
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 25•13 years ago
|
||
pushed to TR-Serrano: 6340:f3b64bc72d18
Updated•13 years ago
|
Attachment #547786 -
Flags: review?(wmaddox) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•