typed array indexing should never consult the prototype

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dherman, Assigned: nmatsakis)

Tracking

({dev-doc-complete})

unspecified
mozilla25
x86
macOS
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Doing a get or set of an indexed property of a typed array should *never* consult the typed array's prototype, even if it's out of range. For example:

    js> Int32Array.prototype[17] = "ehrmagerd"
    js> (new Int32Array(8))[17]
    undefined

Currently, this example produces the string in SpiderMonkey and JSC; V8 produces undefined. The V8 behavior is preferable: it means that getting/setting indexed properties always produces either the typed array's expected type or undefined, and can never trigger getters/setters in the prototype chain.

One way of thinking about this is that getting/setting the indexed property names is overloaded to consult the ArrayBuffer, and never accesses object properties.

(The desired behavior is critical for asm.js to work; it needs to be able to assume that it will never crawl the prototype chain of its buffer, even if the buffer happens to have been neutered and a get/set becomes out of range.)

Dave
Nice, this would benefit IonMonkey too. We have a special MIR instruction that's used if a GetElem ever accessed out-of-bounds typed array elements -- with this change we can mark this instruction as non-effectful, so that we can optimize it, and instead of a VM call we can just load |undefined| if the index is out-of-bounds.
> Doing a get or set of an indexed property of a typed array should *never* consult the
> typed array's prototype

That's not what the spec says for gets on typed arrays right now.  What it says is that sets of out-of-range properties on a typed array should Reject but getting should in fact look up the proto chain.

Furthermore, this behavior is not expressible in WebIDL at the moment, and the typed array spec is apparently trying to be specced in terms of WebIDL (whether it should be is a separate question).

So if want this behavior (which may be fine) we need to change the spec(s) accordingly.

Note that Safari has the same behavior as us; we'd need to get them to change.

Furthermore, note that in Chrome doing this:

<pre><script>
  Int32Array.prototype[200] = "aha";
  var x = new Int32Array(1);
  for (y in x) {
    document.writeln(y + ": " + x[y] + " (own: " + x.hasOwnProperty(y) + ")")
  }
</script>

does show the property, and not as an own property.  So it looks like there's some weirdness there where the property is in fact found on the prototype but the value is controlled by the object itself.  Or something.  Spec it how you will.

Comment 3

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #2)
> > Doing a get or set of an indexed property of a typed array should *never* consult the
> > typed array's prototype
> 
> That's not what the spec says for gets on typed arrays right now.  What it
> says is that sets of out-of-range properties on a typed array should Reject
> but getting should in fact look up the proto chain.

True, if the Kronos TypedArray spec. is assumed to have been written with a deep understanding of the  WebIDL spec. I'm not so sure whether that is the case.  The Kronos spec. is actually rather vague about the semantics of element access. I don't think it really conforms to the WebIDL requirement that "If the operation used to declare the indexed property getter did not have an identifier, then the interface definition must be accompanied by a description of how to determine the value of an indexed property for a given index." I could find no such description for TypedArray.

Regardless, this sort of prototype inheritance of looks-like-but-isnt array lements help no one.  Rather than propagating it, we should kill it in both WebIDL and JS. "If it hurts, don't do it".

> 
> Furthermore, this behavior is not expressible in WebIDL at the moment, and
> the typed array spec is apparently trying to be specced in terms of WebIDL
> (whether it should be is a separate question).

TypedArrays are being subsumed into the ES6 spec.  The current ES6 draft says for them  that all "array index" (approximately, 32-bit positive integers) property keys are treated as indices into the buffer.  Property gets of indices that are out of bound return undefined.  Property sets that are out of bound do nothing. 

(I think it would be even saner to change this so that all numeric property keys are treated as indices.  It really makes no sense to have a property named 1.5 on a TypedArray).


> 
> So if want this behavior (which may be fine) we need to change the spec(s)
> accordingly.

Yes!
> TypedArrays are being subsumed into the ES6 spec. 

Good to know.  If so, it's worth publishing a note to that effect on the Kronos spec so people will know it doesn't reflect reality anymore.

> The current ES6 draft says for them  that all "array index" (approximately, 32-bit
> positive integers) property keys are treated as indices into the buffer.

But getOwnPropertyNames only returns the ones from 0 to length-1, right?  Does enumerating enumerate indexed names on the proto chain?

> Yes!

Sounds like a plan.  I just don't want us doing the thing we have a tendency to do with typed arrays and implement something random without raising a spec issue.  _I_'ve had to raise spec issues on the Kronos spec when it was obviously bogus and our implementation didn't match, and I'd really rather whoever is actually making changes to our implementation actually did that instead.  Basic good web citizenry.
Duplicate of this bug: 855659
(Assignee)

Comment 6

6 years ago
Attachment #772814 - Flags: review?(jdemooij)
Hrm.  I seem to recall out-of-bounds access on typed arrays doing weird stuff in alias analysis because we couldn't tell that it was a typed array access, basically.  Is that fixed with the attached patch?
(Assignee)

Updated

6 years ago
Blocks: 890465
Comment on attachment 772814 [details] [diff] [review]
TypedArrays yield undefined for OOB accesses

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

Very nice, much simpler. As I said in comment 1 (and bz in comment 7) we can also change MLoadTypedArrayElementHole::getAliasSet() to be just like MLoadTypedArrayElement::getAliasSet():

    return AliasSet::Load(AliasSet::TypedArrayElement);

And remove the comment there too of course. r=me with that
Attachment #772814 - Flags: review?(jdemooij) → review+
Once you do that, I would be interested in hearing how this patch affects performance on attachment 675129 [details].
(Assignee)

Comment 10

6 years ago
Carrying over r+ from jandem and incorporating suggested change to alias set. Try run: https://tbpl.mozilla.org/?tree=Try&rev=24ba3db3f59e
Attachment #772814 - Attachment is obsolete: true
Attachment #773488 - Flags: review+
(Assignee)

Updated

6 years ago
Assignee: general → nmatsakis
> I would be interested in hearing how this patch affects performance on attachment 675129 [details] 

Tested, and the answer seems to be "not a huge amount".  About a 5% speedup.
(Assignee)

Comment 12

6 years ago
Carrying over r+ from jandem, minor corrections, try run: https://hg.mozilla.org/try/rev/a8d18785d034
Attachment #773488 - Attachment is obsolete: true
Attachment #773939 - Flags: review+
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/11ffeb44160b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.