Closed Bug 561706 Opened 14 years ago Closed 14 years ago

prepare lookupswitch for fat values

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
JSOP_LOOKUPSWITCH uses indices into the script's atom array to encode the values in case expressions.  This works well for current jsvals since atoms and jsvals have the same representation and thus equality is word comparison (modulo strings and doubles).  In the unboxed 128-bit value world, they have different representations, which means value comparison would be a polymorphic operation (instead of word comparison).  The attached patch simplifies things by instead storing the atom (i.e. jsval) directly in the byte stream.  

An important side effect is that, with the fat value patch, doubles will not need to be atomized at all so the this will remove the last non-trivial case where atoms are not (interned) JSStrings.  So, that much closer to killing all traces of heap doubles and having JSAtom* simply be an (opaque, interned) JSString*.
1. The patch needs comments that the GC rooting of embedding values relies on the values stored in the atom table.

2. There are no js_XDRScript changes.
On performance:

Initially I implemented the bytecode reading/writing macros in terms of GET/SET_UINT32.  This was a distinct slowdown (1-2% slower for earley boyer which has switches and seems not to trace them).  So I just did the obvious (*(jsval *)(pc + 1)) and the slowdown went away.  In fact, SS sped up a bit, mostly in the string tests:

1.007x as fast    630.9ms +/- 0.2%   626.2ms +/- 0.1%     significant

(The speedup makes sense since one level of indirection was removed at the cost of a few bytes.)

It turns out our immediates are always stored big endian.  This raises the question: is there a reason for this?  I would have guessed XDR protects us from having to worry about different architectures reading the same bytecode, if that scenario can even happen.
(In reply to comment #1)
> 2. There are no js_XDRScript changes.

Oh bother, I was just thinking that.
(In reply to comment #2)
> It turns out our immediates are always stored big endian.  This raises the
> question: is there a reason for this?  I would have guessed XDR protects us
> from having to worry about different architectures reading the same bytecode,

I think this conversation came up before - I was told we don't support XDR across platforms. The reason for big-endian encoding was something about reading the stream as words while not wasting space. This is good evidence for not doing that, but soon bytecode reading will be rare so it won't matter as much.
XDR should work across platforms, and I'm pretty sure it's been used that way in the past.
(In reply to comment #4)
> (In reply to comment #2)
> > It turns out our immediates are always stored big endian.  This raises the
> > question: is there a reason for this?  I would have guessed XDR protects us
> > from having to worry about different architectures reading the same bytecode,
> 
> I think this conversation came up before - I was told we don't support XDR
> across platforms. The reason for big-endian encoding was something about
> reading the stream as words while not wasting space.

This doesn't make sense.

Also, XDR (thanks to shaver) uses little-endian order.

Luke was referring to the integral, 16-bits-or-greater immediate operands of bytecodes -- see the GET_UINT16, etc. jsopcode.h macrology. These are highest byte at least address, i.e., big-endian. I'm a Blefuscudian.

/be
There's no particular reason for integral immediates being big-endian, other than to make bytecode hexdump reading easier, btw.

/be
After some more thought, the whole XDR angle really throws a wrench in it.  Currently script->code is just JS_XDRBytes'd, so to bake in pointers I'd have to do patching.  I think WrapEscapingClosure would also need patching.  This is a good bit trickier than the 1-hour patch I posted so I'm thinking WONTFIX and I'll just add more type-specialized loops to JSOP_LOOKUPSWITCH in the same way we already do for strings and doubles.

As for killing non-strings-in-atoms, I realized this can still be accomplished by just giving scripts an array of doubles and having lookupswitch index this table, instead of atomMap, for doubles.  So heap doubles can still die.
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Should we do unaligned loads via the GET_UINT16, etc. macros, though? That sounds like a perf win on modern x86 (it is still not portable, of course, but an #ifdef wouldn't kill us). New bug for sure.

/be
I was thinking of filing a separate bug to spruce up our immediate-reading but, as dvander pointed out above, bytecode interpretation's days are limited so perhaps we can tolerate ntohs and ntohl a little longer.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: