Last Comment Bug 692847 - Speed up Array.shift, and Array.{pop,shift} on empty arrays
: Speed up Array.shift, and Array.{pop,shift} on empty arrays
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla10
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 609835 691314
  Show dependency treegraph
Reported: 2011-10-07 10:32 PDT by Brian Hackett (:bhackett)
Modified: 2011-10-19 03:14 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch against 15bfad783467 (12.61 KB, patch)
2011-10-07 10:32 PDT, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
updated (12.62 KB, patch)
2011-10-07 11:16 PDT, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2011-10-07 10:32:17 PDT
Created attachment 565575 [details] [diff] [review]
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++)
  for (var i = 0; i < 10000000; i++)

js -m -n (old): 625
js -m -n (new): 322
js -m -j -p: 805
d8: 592
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2011-10-07 11:05:25 PDT
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 User image Brian Hackett (:bhackett) 2011-10-07 11:16:53 PDT
Created attachment 565591 [details] [diff] [review]

Oops, was missing a coercion that was necessary on x64.
Comment 3 User image Brian Hackett (:bhackett) 2011-10-18 11:09:29 PDT
Comment 4 User image Andrew McCreight [:mccr8] 2011-10-18 11:12:06 PDT
As an aside, according to Peacekeeper doesn't do a bunch of operations on empty arrays any more, in arrayCombined.
Comment 5 User image Marco Bonardo [::mak] 2011-10-19 03:14:39 PDT

Note You need to log in before you can comment on or make changes to this bug.