nanojit: make LirReader::read() clearer and faster

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 397814 [details] [diff] [review]
patch

With LIR_2 gone, LirReader::read() can be reworked to be clearer and faster.
Specifically:

- Currently the loop never has a skip on the first iteration, but only has
  skips on the 2nd and subsequent iterations.  So I split it into a single
  switch (which doesn't have to handle skip) followed by a skip-chasing
  loop.  This makes things clearer.

- The LIR_start case can be skipped in optimised builds, avoiding one
  comparison.  Doesn't make a noticeable difference to SunSpider, but it
  can't hurt in the long run.

- Added some comments making some invariants clearer.

- Removed setpos(), which is dead.
Attachment #397814 - Flags: review?(gal)

Comment 1

8 years ago
Comment on attachment 397814 [details] [diff] [review]
patch

Cool.
Attachment #397814 - Flags: review?(gal) → review+

Comment 2

8 years ago
I wonder whether its worth grouping the calls such that we can mask-match them. Just an idea.
(Assignee)

Comment 3

8 years ago
I don't understand -- what do you mean by "mask-match"?

It's a shame that calls are variable-length.  I wonder if making them fixed length -- unused arguments would just be wasted space -- would be worth it just to avoid this switch.

Comment 4

8 years ago
icall is 16. fcall is 16 | 64 and qcall is 17 | 64. So

((iop &~ (64 | 1)) == 16)

should be equivalent. This would need some asserts and explanation in LIRopcode.tbl and blocking of 17 (which is currently unused anyway).

Its a bit ugly, but might be fast.
(Assignee)

Comment 5

8 years ago
Hmm, I'll believe it's faster when I see the numbers...

Comment 6

8 years ago
Yeah, its only worth taking that ugly hack if it is actually measurably faster.
(Assignee)

Comment 7

8 years ago
http://hg.mozilla.org/tracemonkey/rev/48928150aa27
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 8

8 years ago
Backed out due to Tinderbox crashes:

http://hg.mozilla.org/tracemonkey/rev/0fc46fb6cba1
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 9

8 years ago
Profiling under 20x-over-compilation (see bug 504258) has shown a very slight slowdown.  If bug 515305 is completed that will supersede this anyway.
http://hg.mozilla.org/mozilla-central/rev/48928150aa27
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/0fc46fb6cba1 -- oops.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

8 years ago
Created attachment 411614 [details] [diff] [review]
new patch

Thanks to bug 525437, which removed the special-casing required for calls, this is back on the table.  For an optimised build we now just (a) do a table-lookup to get the instruction size, step back that many bytes, and (b) check if we're now on a skip, and if so follow it back to the previous chunk.  No switches required!
Attachment #397814 - Attachment is obsolete: true
Attachment #411614 - Flags: review?(edwsmith)
(Assignee)

Updated

8 years ago
Attachment #411614 - Flags: review?(edwsmith) → review?(gal)

Updated

8 years ago
Attachment #411614 - Flags: review?(gal) → review+

Comment 13

8 years ago
tamarin has code that erases instructions by turning them into a LIR_skip, but that could cause read() to see two skips in a row.  so, maybe don't want that assert there at least until tamarin's erasure code is changed.
(Assignee)

Comment 14

8 years ago
Ed, I'll change the skip test to a while loop that ignores multiple skips in a row.
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/projects/nanojit-central/rev/d78bd673c865
Whiteboard: fixed-in-nanojit
(Assignee)

Comment 16

8 years ago
http://hg.mozilla.org/tracemonkey/rev/b45ea32fad3e
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey

Comment 17

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b45ea32fad3e
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

8 years ago
For the record, this introduced an intermittent bug :(  It was fixed in bug 530713.
You need to log in before you can comment on or make changes to this bug.