514110 - nanojit: avoid getresv() because it has a redundant test

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

We see this pattern a lot in the Assembler:

    Reservation* resv = getresv(ins);
    NanoAssert(resv);
    ... use resv->reg, etc ...

getresv() is inlined, so it becomes this:

    Reservation* resv = ins->resv();
    resv = (r->used ? r : 0);
    NanoAssert(resv);
    ... use resv->reg, etc ...

In optimised builds the assert is removed:

    Reservation* resv = ins->resv();
    resv = (r->used ? r : 0);
    ... use resv->reg, etc ...

The r->used test should always succeed (if it doesn't, there's a bug which
a debug build should catch).

This patch removes all occurrences of this idiom in Assembler.cpp and
Nativei386.cpp (but not the other back-ends), replacing them with this:

    Reservation* resv = ins->resv(ins);
    NanoAssert(resv->used);
    ... use resv->reg, etc ...

thus avoiding the unnecessary test.  (A couple of the modified places didn't have the assertion, but the code relied on resv->used being true.)

Furthermore, I introduced resvUsed() which does the assert itself, so now 
you actually see this:

    Reservation* resv = ins->resvUsed(ins);
    ... use resv->reg, etc ...

I also changed other "resv = getresv(ins)... if (resv)" occurrences to 
"resv = ins->resv()... if (resv->used)" for consistency and readability.

I've run SS several times and am seeing a 0--3ms speedup.  I also checked 
the generated asm and it's several instructions fewer for each occurrence 
and so is a clear win in terms of code size.

Comment 1

[Andreas Gal :gal]

Looks great, but I am not a big fan of the resvUsed() name. I think I prefer the explicit assert. It helps reading the code if its explicit there, at least for me. If you find a better name for resvUsed, I would be ok with that too. Any ideas?

Comment 2

[Nicholas Nethercote [inactive]]

Earlier I was using resv() for the checking version, and resvMaybeUnused() for the unchecked version -- is that any better?

Or if you want more verbosity: getUsedResv()?

Comment 3

[Andreas Gal :gal]

getUsedResv sounds pretty cheesy too. I don't have any great ideas either though. resvOrNull? I still like resv() and explicit asserts best.

Comment 4

[Nicholas Nethercote [inactive]]

resvOrNull() matches the old behaviour -- resvOrAssert() would be more appropriate.

The reason I want to remove the assertion is that if you look through Assembler.cpp and the back-ends there is an amazing amount of code dealing with reservations when really a reservation is a middle-man -- what we're really interested in is the reg/used/arIndex fields.  I want to reduce some of this noise, it obscures the interesting work done by the code.  Maybe functions like ins->getReg() and ins->setReg() would be better.

But for the moment I'll probably just put the asserts back in.

Comment 5

[Andreas Gal :gal]

Yeah, I agree. getresv() is a leftover of when there was a mapping table in between. Maybe we should get rid of the Reservation struct.

Comment 6

[Andreas Gal :gal]

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

Looks good. Handle resv*() as you see fit. All options are about equally ugly.

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/4911291fc158

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/4911291fc158

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9f63d8565bff