515138 - TM/nanojit: merge the two StackFilter passes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

StackFilter is a stage that occurs twice in the reader pipeline, once each for SP and RP.  In each stage, every instruction is tested to see if it's a store with certain properties, in which case it's omitted.  Each instruction is also tested to see if it's a guard, in which case some state is updated.

Bug 514062's data shows StackFilter::read() is one of the most expensive single functions in all of NJ.

This patch merges the two stages into one, avoiding a read() call and the isStore/isGuard() for every instruction.  I'm currently seeing about a 5ms speedup for SunSpider which I don't entirely trust, but 20x-over-compiling (see bug 504258) agrees and Cachegrind says fewer instructions are being executed so it looks like a clear win.

Comment 1

[Nicholas Nethercote [inactive]]

BTW, I gathered some stats about how often all the ignoreStore() sub-cases occurred to make sure I was preserving the current behaviour.  They're gone from the submitted patch, but they mean I'm pretty confident about the change.

Comment 2

[Andreas Gal :gal]

Comment on attachment 399172 [details] [diff] [review]
patch

> 
>         // STOREFILTER for sp
>-        StackFilter storefilter1(prev, alloc, frag->lirbuf, frag->lirbuf->sp);
>-        prev = &storefilter1;
>+        StackFilter stackfilter1(prev, alloc, frag->lirbuf, frag->lirbuf->sp, frag->lirbuf->rp);
>+        prev = &stackfilter1;

This can be storefilter now, right?

>-                stk.reset();
>-                top = getTop(i) >> 2;
>+                spStk.reset();
>+                rpStk.reset();
>+                spTop = getSpTop(i) >> 2;
>+                rpTop = getRpTop(i) >> 2;

we should make this getTop(i, &spTop, &rpTop) imo.

Otherwise looks great. Please make sure Ed is cc'd but I am sure they don't mind. They don't use the code and this makes them faster too and removes code, so a win all around.

Comment 3

[Andreas Gal :gal]

Last I profiled this I don't think we ever remove any rp stores (but I was hoping to take advantage of this soon-ish with more guard elimination). If we mess up sp store removal you would run into test failures. We went through this pain before and added some tests.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> (From update of attachment 399172 [details] [diff] [review])
> > 
> >         // STOREFILTER for sp
> >-        StackFilter storefilter1(prev, alloc, frag->lirbuf, frag->lirbuf->sp);
> >-        prev = &storefilter1;
> >+        StackFilter stackfilter1(prev, alloc, frag->lirbuf, frag->lirbuf->sp, frag->lirbuf->rp);
> >+        prev = &stackfilter1;
> 
> This can be storefilter now, right?

I changed the variable name from storefilter to stackfilter to match the type.  I'm not fussed either way but whichever we choose we should be consistent IMO.

Comment 5

[Edwin Smith]

(In reply to comment #2)

> Otherwise looks great. Please make sure Ed is cc'd but I am sure they don't
> mind. They don't use the code and this makes them faster too and removes code,
> so a win all around.

Yup, looks good to me.

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/25432eb55415

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/25432eb55415

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/940a2268be85