692847 - Speed up Array.shift, and Array.{pop,shift} on empty arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch against 15bfad783467

Peacekeeper 2.0's arrayCombine test is broken and does many of its operations on an empty array, with a bunch of pop and shift calls costing the most.  V8 has a fast path for pop() on an empty array, so I don't feel too bad benchmark-gaming this one.

Attached patch allows the pop() inline path to handle empty arrays (only when the call is known to produce undefined), and generalizes this path to also handle shift() (which calls a stub to memmove() in the non-empty case).  This gives a good speedup to non-empty shift() too:

function foo(x) {
  for (var i = 0; i < 10; i++)
    x.push(i);
  for (var i = 0; i < 10000000; i++)
    x.push(x.shift());
}
foo([]);

js -m -n (old): 625
js -m -n (new): 322
js -m -j -p: 805
d8: 592

Comment 1

[Boris Zbarsky [:bzbarsky]]

Brian, I get an error about ‘js::mjit::Assembler::storeValueFromComponents(JSValueType&, JSC::X86Registers::RegisterID, JSC::AbstractMacroAssembler<JSC::X86Assembler>::Address&) not being defined when I build this on top of rev 15bfad783467.  This is in a 64-bit build.  A 32-bit build works fine.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: updated

Oops, was missing a coercion that was necessary on x64.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/882404096d65

Comment 4

[Andrew McCreight [:mccr8]]

As an aside, according to https://bugzilla.mozilla.org/show_bug.cgi?id=499198#c32 Peacekeeper doesn't do a bunch of operations on empty arrays any more, in arrayCombined.

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/882404096d65