Dave Herman [:dherman] Reporter
	Description • 11 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

	Jan de Mooij [:jandem]
	Comment 1 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 11 years ago

> 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.

	Allen Wirfs-Brock
	Comment 3 • 11 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!

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

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?

	Jan de Mooij [:jandem]
	Comment 8 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

Once you do that, I would be interested in hearing how this patch affects performance on attachment 675129 [details].

	Niko Matsakis [:nmatsakis] Assignee
	Comment 10 • 11 years ago

Carrying over r+ from jandem and incorporating suggested change to alias set. Try run: https://tbpl.mozilla.org/?tree=Try&rev=24ba3db3f59e

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

> 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.

	Niko Matsakis [:nmatsakis] Assignee
	Comment 12 • 11 years ago

Carrying over r+ from jandem, minor corrections, try run: https://hg.mozilla.org/try/rev/a8d18785d034

	Niko Matsakis [:nmatsakis] Assignee
	Comment 13 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/11ffeb44160b

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/11ffeb44160b

	Florian Scholz (Open Web Docs)
	Comment 15 • 10 years ago

Added a notes to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray#Indexed_property_access
https://developer.mozilla.org/en-US/Firefox/Releases/25#JavaScript