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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: vlad, Assigned: froydnj)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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!
Hmm.  This regressed between 14 and 15.

There were some typed array changes in there, definitely (bug 741039, bug 575688, bug 741041).
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
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....
Attached patch patch (obsolete) — Splinter Review
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...
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...
Attached patch patchSplinter Review
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
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 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?
(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.
You could always do both ;)
(In reply to Tom Schuster [:evilpie] from comment #10)
> You could always do both ;)

What is "both" in this context?
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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3244b8ba1b7
Assignee: general → nfroyd
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/d3244b8ba1b7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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.

Attachment

General

Created:
Updated:
Size: