Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Spec text doesn't throw on non-Number lane indices #308

Closed
nmostafa opened this issue Dec 15, 2015 · 9 comments
Closed

Spec text doesn't throw on non-Number lane indices #308

nmostafa opened this issue Dec 15, 2015 · 9 comments

Comments

@nmostafa
Copy link
Contributor

In #120, we agreed to accept only Numbers as lane indices.

1.Throw an exception for the following cases:
Any lane index is a not a Number type.

Looking at the spec text, I don't see this being enforced.

This is reflected in couple of places in the spec.

  1. SIMDToLane. It coerces any type to number. And only throws if out of range.
SIMDToLane( max, lane )#
1.Let index be ToNumber(lane).
2.ReturnIfAbrupt(index).
3.If SameValueZero(index, ToLength(index)) is false or index < 0 or index ≥ max, throw a RangeError exception.
4.Return index
  1. SIMDConstructor.shuffle( a, b, ...lanes ) and swizzle. Assumes 0 for missing lane indices. Missing lane indices are of Undefined value, and we should throw here.
3.For integer n from 0 to SIMDDescriptor.[[VectorLength]],a.Let lane be lanes[i], or 0 if lanes is not long enough.
@ljharb
Copy link
Member

ljharb commented Dec 15, 2015

My understanding is that "only accepting number literals as indices" is an asm.js optimization requirement - there's no reason the functions can't or shouldn't accept numbers via a variable. It's very typical for builtin methods that take numbers to perform ToNumber on the item, and I think the SIMD methods should do that too - even though it wouldn't be necessary in the asm.js subset.

@nmostafa
Copy link
Contributor Author

The builtin should accept a Number variable outside Asm.js. That part is fine. The concern is allowing non-Numbers to be passed as lane indices. Simd.js builtins is all about performance, so whenever possible, we should make the API strict to avoid hidden perf cliffs (especially if the flexibility is not justified by existing use-cases).

In this case JS allows it, and we should throw TypeError. Otherwise, the type-specializer will give up on these cases, and either bailout or call a helper, either of which is very slow.

@nmostafa
Copy link
Contributor Author

And by the way, we allowed lane indices to be non-literals, just because there is no way in JS to enforce that outside Asm.js. SIMD.js documentation should encourage use of literals, otherwise perf is not guaranteed.

@jfbastien
Copy link

I agree with @nmostafa: simd.js is solely a performance feature, the API should IMO reflect this as much as possible and avoid "it just works" behavior whenever possible.

@littledan
Copy link
Member

We changed the throwing to a cast at the direct request of TC39. I don't think this changes performance at all; it just makes it more consistent with the rest of the language. ToNumber is all over JavaScript, and accordingly, VMs already optimize it well. I think we discussed this change in a phone call (but of course it's fine to revisit). I don't think this will affect either asm.js or non-asm.js performance.

@littledan
Copy link
Member

@nmostafa Why do you think this would make a perf cliff?

@nmostafa
Copy link
Contributor Author

ToNumber would be optimized well, if the argument is a Number value. If given a String, Array or Bool, for example, it will require special handling via calling a helper. VMs are very unlikely to handle these cases via a fast-path, since they are very rare to happen.

Now, if a developer has a bug that inadvertently assigns a non-Number to a lane index, he will face mysterious perf degradation. It would be nicer if the code throws a type-error exposing the bug.

All SIMD ops have statically typed operands, I don't see why lane indices should be any different.

@littledan
Copy link
Member

Nothing is statically checked for; everything is dynamic. Only SIMD types are checked for with a TypeError. Everywhere in the spec, when a Number is required, we call ToNumber. This is just the inheritance from being embedded in the rest of JS. Sure, it will be slower if you pass a non-Number, but this isn't really a cliff so much as a "pay-as-you-go" language feature as far as I can tell. The only cliff is that you lose asm.js validation, but this is a cross-cutting design decision for asm.js.

We are at Stage 3 in TC39. That means the committee has signed off on our aesthetic choices, and any further changes should be driven by feedback from implementation concerns, not design-level feedback. It took a lot of discussion and compromises to get here; some committee members wanted much more drastic changes which would've really hurt performance in mainline cases. I don't think this hurts anyone's performance, and it doesn't cause a cliff but rather an incremental cost which is exactly parallel to every single other place in JS where Numbers are accepted as arguments. Developers can also use systems like TypeScript to avoid this hazard, just like they would elsewhere in the language.

I don't think reopening this issue would have practical benefit. It is likely to make things more difficult in TC39 (and we need their continued buy-in to get follow-on features like Float64x2 and saturatingAbsoluteDifference), and it is unlikely to make a big difference for developers. Getting rid of some of JavaScript's implicit coercions could definitely have some benefits, but @rossberg-chromium has been arguing this for a while, and it seems the fight has been lost.

@johnmccutchan
Copy link
Collaborator

Unfortunately, JS is full of performance cliffs. This issue has already been discussed at length and at the committee's request we have the existing design. The expectation is that performance analysis tools will help developers identify this specific performance cliff if they hit it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants