Closed Bug 609896 Opened 9 years ago Closed 6 years ago

array_concat should be using a fast path for dense arrays

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bzbarsky, Assigned: terrence)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

Copying via memcpy instead of GetArrayElement/SetArrayElement, say!
Blocks: 609835
I'll take this. I'm doing similar stuff in bug 581180.
Assignee: general → pbiggar
I don't think you can quite do memcpy (holes in dense arrays...).

Paul: I just added a GetElements() function in jsarray.cpp that I think could be reused here.
(In reply to comment #2)
> Paul: I just added a GetElements() function in jsarray.cpp that I think could
> be reused here.

Yes, looks like it. Aside: what a curious name you chose. It's called 'Get' but it actually copies into its 4th parameter...
Yes, it's the vector form of GetElement, which takes a scalar Value outparam.
Hmm.  Why can one not memcpy holes, exactly?
(In reply to comment #5)

js> Array.prototype['3'] = "surprise"
"surprise"
js> ["hi"].concat([1,2,3,,5])
["hi", 1, 2, 3, "surprise", 5]
We should just check obj->isDenseArray() && !js_PrototypeHasIndexedProperties(cx, obj) to handle that.

Might also need some sort of check on how length compares to capacity; other array functions have those.
Awesome.  Can the js_PrototypeHasIndexProperties optimization can be put in GetElements (which is hot in, e.g., v8-raytrace)?
GetElements doesn't seem to get holes off the proto.  Am I missing something?
You are correct sir!  This is a regression, will file.
Luke, did you file this? I can't find it.
This is nasty code with nasty edge cases, but it would look much nicer once we have Lua-style arrays, so waiting til after that.
Assignee: pbiggar → general
Depends on: 586842
Depends on: 688852
Assignee: general → terrence
Status: NEW → ASSIGNED
This ~doubles the perf on the attached benchmark and exposes a bug.  Go read 698140 if you want to know more.
Depends on: 698140
No longer depends on: 586842
Blocks: 797401
Terrence, can we land your patch? I'd love to have this in for bug 609835.

It probably needs a number of changes though, for instance passing obj->getDenseElements() to InitArrayElements will make GGC unhappy.
Flags: needinfo?(terrence)
Yes, absolutely! I must have forgotten about this, let me rebase it.
Flags: needinfo?(terrence)
With a clang3.3 opt build on the attached micro-benchmark:
  before: 6.53s
  after:  1.48s
Attachment #570407 - Attachment is obsolete: true
Attachment #809360 - Flags: review?(jwalden+bmo)
(In reply to Jan de Mooij [:jandem] from comment #16)
> Terrence, can we land your patch? I'd love to have this in for bug 609835.
> 
> It probably needs a number of changes though, for instance passing
> obj->getDenseElements() to InitArrayElements will make GGC unhappy.

Oh, this isn't going to be unsafe: the actual memcpy has been gone for awhile now. The dense->dense path loops over elements and calls HeapSlot::set or init on each element. Now that I think about it, we should easily be able to hoist the barriers here and restore the memcpy. I'll test and whip up a patch.
Without ggc, this is not a significant savings. With ggc our time goes from 1.8s to 1.6s.
Attachment #810177 - Flags: review?(wmccloskey)
Attachment #810177 - Flags: review?(wmccloskey) → review+
Comment on attachment 809360 [details] [diff] [review]
array_concat_dense_fastpath-v1.diff

Review of attachment 809360 [details] [diff] [review]:
-----------------------------------------------------------------

This looks the same as the patch in bug 688852, no?  Same issues as there.
Attachment #809360 - Flags: review?(jwalden+bmo)
Whiteboard: [leave open
Terrence: did you intend to leave this bug open after you landed opt_copy_init_dense_elems-v0.diff?
Flags: needinfo?(terrence)
Yeah, I don't think there's more that needs to be done here unless we have some new test case.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(terrence)
Resolution: --- → FIXED
Whiteboard: [leave open
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.