520590 - TM: RecordLoopEdge mis-aborts on inner tree

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

RecordLoopEdge, which is called when an inner loop edge is encountered, will abort if the inner loop is encountered for the first time, and for the wrong reason:

js> for (var i=0; i<10; ++i) { if (i>=1) for (var k=0; k<10; ++k) {} } 
Abort recording of tree typein:2@15 at typein:2@32: Loop edge does not return to header.

(Here, recording starts when i == 1.)

The reason is that "getLoop()" is used to lookup an existing fragment, which does not attempt to create a VMFragment at the (pc,global,shape,argc) key if one does not exist).

The fix is to use something like "getAnchor()" that creates a Fragment if one does not already exist.  With bug 519567, the outer recorder can be suspended and the inner recorder can begin immediately.  This reduces aborts and time spent in the interpreter.

Comment 1

[Luke Wagner [:luke]]

Also TODO on this patch: avoid redundant Fragment lookup via getLoop and getAnchor.

Comment 2

[Graydon Hoare :graydon]

On the one hand, I would say "yay!" and encourage this to proceed. Likewise on the further TODO. In fact, the whole fragment-lookup hash is probably worth replacing with a more reasonable dynamic hashtable.

On the other hand, Be Careful Here. Or perhaps Be Careful Accepting Anything I Say On The Matter. Talk to dvander. The logic about validity of knitting trees together -- when you can or should start a new tree, when a treecall is valid or not -- is frequently surprising to me. There are details I fail to recall or comprehend.

Comment 3

[Luke Wagner [:luke]]

(In reply to comment #2)
Indeed this bug was preceded by a discussion with David :).  I should cc him.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix

This patch fixes the bug.  Neither getLoop nor getAnchor do quite what is needed.  Furthermore, getLoop and getAnchor do redundant computation, so I refactored these functions and their callers a bit.  This results in simpler code, but no measurable performance difference on SunSpider or v8.

Comment 5

[Luke Wagner [:luke]]

Attachment: oops, real one

Oops, that last one was out of date, and I forgot to mention:
 - with dmandelin's help, I tweaked trace-test.py to print out the current executing test if trace-tests is interrupted
 - I found a small bug in CheckGlobalObjectShape where 'false' was returned without resetting the JIT.  RecordLoopEdge depends on this.  Added explanatory comments.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 404928 [details] [diff] [review]
oops, real one

Can we rename LookupAddLoop to LookupOrAddLoop?

r=me with that.

Comment 7

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/ca95f2397bc5
and then
http://hg.mozilla.org/tracemonkey/rev/c781672ff5cb
because I forgot to include dvander's rename and I'd like to pull in a tiny bugfix I found working on something else.

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ca95f2397bc5