Closed
Bug 786903
Opened 12 years ago
Closed 12 years ago
new TypedArray(otherTypedArray) is way slower than it should be
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: vlad, Assigned: froydnj)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
1.88 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
Via http://jsperf.com/float32array-initialization -- the "Init from typed array case" really shouldn't be slower than the "Init from array" case. But it's much slower -- 2,432,581 ops/sec vs. 891,107 for the typed array case!
Comment 1•12 years ago
|
||
Hmm. This regressed between 14 and 15. There were some typed array changes in there, definitely (bug 741039, bug 575688, bug 741041).
Comment 2•12 years ago
|
||
We _used_ to have a fast-path for this constructor. This was removed in bug 741041, looks like. Specifically, see the removal of the isTypedArray() case in create(). The new code does this: 3.183 + if (!UnwrapObject(dataObj)->isArrayBuffer()) 3.184 + return fromArray(cx, dataObj); but fromArray calls GetLengthProeprty and then calls copyFromArray, and copyFromArray does this: if (ar->isDenseArray() && ar->getDenseArrayInitializedLength() >= len) { // fast code using getDenseArrayElements() } else { // slow code using JSObject::getElement } Sounds like we might want a typed array fast-path in copyFromArray?
Blocks: 741041
Keywords: regression
Comment 3•12 years ago
|
||
Alternately, we could use our existing copyFromTypedArray fast-path, which is what we used to do, in the case when dataObj is a typed array....
Assignee | ||
Comment 4•12 years ago
|
||
This patch adds the typed array fast path bz suggested, but init'ing from a typed array is still very slow. It's possible that this is because I was testing on a debug build, but still, peculiar. Going to try an opt build...
Assignee | ||
Comment 5•12 years ago
|
||
An unpatched opt build shows that the typedArray initialization path is about 50% slower; a patched opt build improves that to ~30% slower, but we're still not back to FF 14 levels yet. And it's still slower than initializating from a garden-variety array...
Assignee | ||
Comment 6•12 years ago
|
||
OK, so this patch gets us within about ~10% of the fastest time and makes typed array initialization a bit faster than normal array initialization.
Attachment #656860 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 656897 [details] [diff] [review] patch Hm, I tried to move all the checks for typed-ness inside fromArray, but I was getting weird crashes in the JS allocator. So we'll just ask for review on this version instead.
Attachment #656897 -
Flags: review?(sphink)
Comment 8•12 years ago
|
||
Comment on attachment 656897 [details] [diff] [review] patch Review of attachment 656897 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jstypedarray.cpp @@ +1726,5 @@ > fromArray(JSContext *cx, HandleObject other) > { > uint32_t len; > + if (other->isTypedArray()) { > + len = length(other); TypedArrays "length" property is actually well known, why don't we move this into GetLengthProperty? Maybe even have a follow up to optimize GetElement for it?
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #8) > ::: js/src/jstypedarray.cpp > @@ +1726,5 @@ > > fromArray(JSContext *cx, HandleObject other) > > { > > uint32_t len; > > + if (other->isTypedArray()) { > > + len = length(other); > > TypedArrays "length" property is actually well known, why don't we move this > into GetLengthProperty? Testing this idea reveals that the isTypedArray check must come before the isArray check in GetLengthProperty for this to not hurt the benchmark speeds. If making all array.length accesses slower for the nicer code is worth it...I submit to the will of the JS reviewers in any event.
Comment 10•12 years ago
|
||
You could always do both ;)
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #10) > You could always do both ;) What is "both" in this context?
Comment 12•12 years ago
|
||
Put it in GetLengthProperty, after the isArguments, but also keep the inline version in fromArray. But this is not very important, so file a bug if you want.
Comment 13•12 years ago
|
||
Comment on attachment 656897 [details] [diff] [review] patch Review of attachment 656897 [details] [diff] [review]: ----------------------------------------------------------------- Whoops. Thanks for putting this back. I'm ambivalent about the typed array length specialization. I'd expect it to be relatively infrequently called, so probably not worth the extra few lines of code. But that's another bug anyway.
Attachment #656897 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3244b8ba1b7
Assignee: general → nfroyd
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d3244b8ba1b7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Reporter | ||
Comment 16•12 years ago
|
||
Any place where testcases like this are collected for automated regression testing?
You need to log in
before you can comment on or make changes to this bug.
Description
•