509093 - clean up joinEdgesToEntry

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: proposed changes

Patch cleans up joinEdgesToEntry.  Preserves functionality except for one detail: it tries to link trees together that have missing globals, but their minimal subset still matches. The old code only matched trees that had completely identical typemaps which is unnecessary.

Comment 1

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

blocks recursion after all.

Comment 2

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

Attachment: v2

sayrer noted the control flow in the original patch was still funky. this patch removes the break and continue.

Comment 3

[Andreas Gal :gal]

Comment on attachment 393935 [details] [diff] [review]
v2

JS_MIN might call the getters twice. Might want to copy them to variables first.

Maybe split the function into a smaller one for the inner loop? The break is hard to follow. Just a nit. Implement at your leisure.

Comment 4

[Robert Sayre]

Comment on attachment 393935 [details] [diff] [review]
v2

yeah, I agree with Andreas, but I can live with it.

OTOH, it looks like 

Queue<unsigned> demotes;

is declared too early, and has setLength called where it needn't be.

Comment 5

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

(In reply to comment #4)

I did this to avoid reallocating it on every iteration. If no one cares about that I'll move it down.

Comment 6

[Brendan Eich [:brendan]]

Explicit hoisting of an allocation out of a loop could be good -- worth a comment at any rate.

/be

Comment 7

[Robert Sayre]

yeah, small thing. I can't really tell if there will be a lot of loops that never need the demotes Queue, and it might not matter at all.

But the setLength() call at the beginning of the while loop can go, right?

Comment 8

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

Attachment: v3

splits some stuff out of the loop, removes redundant setLength()

Comment 9

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

http://hg.mozilla.org/tracemonkey/rev/7a1032f3aac0

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7a1032f3aac0

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e0d686f8310b