Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The new Vector relies on catch AIOOBEs, which are UB in Scala.js #11920

Closed
sjrd opened this issue Mar 26, 2020 · 1 comment
Closed

The new Vector relies on catch AIOOBEs, which are UB in Scala.js #11920

sjrd opened this issue Mar 26, 2020 · 1 comment
Assignees
Labels
Milestone

Comments

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 26, 2020

See https://contributors.scala-lang.org/t/new-vector-performance-on-scala-js/4135 for some context. The new Vector implementation relies on catching ArrayIndexOutOfBoundsExceptions happening on its internal arrays, to then throw user-level exceptions.

This doesn't work on Scala.js, because AIOOBEs are Undefined Behavior (they have always been). This means that accessing out of bounds of a Vector now throws an UndefinedBehaviorError with internal details of Vector, instead of user-level IndexOutOfBoundsException (mentioning the size of the vector, not the size of some internal array) or NoSuchElementException.

I have already sent a PR to fix the two NoSuchElementExceptions (scala/scala#8827), because NSEEs really don't have any precedent for being UB. There are still a bunch of cases to throw IndexOutOfBoundsExceptions. Those are less bad but they're still pretty bad, as index out of bounds on Scala collections have never been UB before.

@lrytz lrytz added this to the 2.13.2 milestone Mar 26, 2020
@lrytz lrytz added the blocker label Mar 26, 2020
@sjrd

This comment has been minimized.

Copy link
Member Author

@sjrd sjrd commented Mar 26, 2020

Also, all those try..catch'es may induce significant performance issues in Scala.js. Even when no exception is actually thrown and caught, a try/catch is bad for performance in most JS engines (first and foremost V8, which is in Node.js and Chrome).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.