490947 - nanojit: remove reservation table

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

From comment 6 in bug #488775, in relation to the fact that we will have more space in LIR instructions once that patch has landed:

"We can make the reservation table go away in a follow up patch since we have more room now. Benefits:

1) failed assembly doesn't leave LIR in a bad state
2) no more running out of reservation space
3) LIR tells us where values live (register/stack), which can be used to make
branch code more efficient (read values straight from the registers instead of
going via memory)"

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch removing resv table

(In reply to comment #0)
>
> 1) failed assembly doesn't leave LIR in a bad state
> 2) no more running out of reservation space
> 3) LIR tells us where values live (register/stack), which can be used to make
> branch code more efficient (read values straight from the registers instead of
> going via memory)"

The attached patch is a start:  it removes the resvTable, putting Reservation information into LIns.  Currently it's a bit ugly;  I had to put the opcode in the Reservation struct in order to keep the LIns size at 4 words, I couldn't work out how else to do it.  (One possibility would be to remove the Reservation struct altogether, and just deal with the arIndex/reg/used fields directly, but that would require modifying every caller of getresv(), of which there are many.)

In terms of the benefits above, the patch definitely achieves (2), I'm not sure about (1), and it doesn't even try to do (3) because I don't know how to do that.  Andreas, can you explain further?

Comment 2

[Andreas Gal :gal]

Comment on attachment 376677 [details] [diff] [review]
patch removing resv table

Reservation index used to clobber LIR no longer the case with this patch. Can you flag Ed or rick for review too?

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: updated patch, with bits of #492319 in it

This patch ended up almost entirely subsuming the patch attached to #492319.  So I've updated the patch here to include the handful of changes (some added comments and padding removal in LIns) that were in #492319's patch but not here.  Those changes have already been approved by Ed so the new patch shouldn't need reviewing.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 376816 [details] [diff] [review]
updated patch, with bits of #492319 in it

Oh, except that Andreas asked that I get Ed to review it.  Request added.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: SunSpider results

This change possibly gives a slight performance improvement.  Attached results are from a SunSpider run with --runs=500... I needed that many runs to get the variance down close to 1%.  It's still not enough for the SunSpider script to bless it with overall statistical significance, but it's better to have gained an unreliable 9ms than lost an unreliable 9ms :)

Comment 7

[Edwin Smith]

Comment on attachment 376816 [details] [diff] [review]
updated patch, with bits of #492319 in it

Assembler.cpp:661, comment needs updating.

looks nice. Agree it's ugly putting opcode in Reservation, but materially doesnt matter as long as we use LIns accessor methods to touch opcode.  

and +1 for less code churn.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: rebased patch

Rebased patch.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/1cfa8fa97970

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1cfa8fa97970