Closed Bug 609896 Opened 9 years ago Closed 6 years ago
_concat should be using a fast path for dense arrays
7.68 KB, patch
|Details | Diff | Splinter Review|
3.04 KB, patch
|Details | Diff | Splinter Review|
Copying via memcpy instead of GetArrayElement/SetArrayElement, say!
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
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.
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.
Yes, absolutely! I must have forgotten about this, let me rebase it.
With a clang3.3 opt build on the attached micro-benchmark: before: 6.53s after: 1.48s
(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.
Comment on attachment 810177 [details] [diff] [review] opt_copy_init_dense_elems-v0.diff https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbafd515595
Attachment #810177 - Flags: checkin+
Terrence: did you intend to leave this bug open after you landed opt_copy_init_dense_elems-v0.diff?
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
Resolution: --- → FIXED
Whiteboard: [leave open
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.