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

Semantics suggestion: Use SpeciesConstructor just for new.target #1

Open
littledan opened this issue May 21, 2020 · 9 comments
Open

Comments

@littledan
Copy link
Member

littledan commented May 21, 2020

Great writeup! I'm really excited about reconsidering this stuff. Two things occur to me:

  • People may still have the intuition that a subclass instance is constructed, when you subclass Array and call .map on it. But this use of subclassing might mostly be for methods, not for constructor behavior
  • The Buffer polyfill thing has the risk of being a real web compatibility issue. At least, the pre-ES6 versions of Buffer polyfills had web compat issues, leading to tweaks and delays in shipping full ES6 semantics IIRC (too lazy to find references ATM; press me on this if you need more info).

These might both be solved with a simple tweak: Rather than totally abandoning the this.constructor[@@species] pattern, what if we continue to use it, but only to determine the new.target, and then we still go through the built-in constructor codepath? Similarly, for static methods, you can use this as the new.target but still go through the original constructor path. Some implications:

  • The most visible compatibility/intuition issues go away, since you get subclass instances, @@species still exists on the same things, etc
  • You still have an original Array/RegExp/etc instance in the places you expect. There's no "code injection" in paths that construct and then manipulate things.
  • I think implementations would still want a species protector, to avoid the cost of doing the actual SpeciesConstructor calculation (since it may be implemented in a part of the engine that can't benefit from ICs)
  • Although it's a little unfortunate that subclasses can't put in initialization logic, this decision would not use any "superpowers" since Reflect.construct is already a thing--you already can't count on things whose __proto__ is something to have gone through a particular instantiation path.
@littledan
Copy link
Member Author

littledan commented May 21, 2020

Note that, a downside of this proposal which @jandem mentioned is that, you still have two Get's, for this.constructor and then [Symbol.species], which have arbitrary side effects. Inserting these arbitrary side effects in the middle of previously simpler code caused security issues in the past in some engines. So, from an implementation perspective, this only solves some of the problems, having to do with processing after instantiation.

@syg
Copy link
Collaborator

syg commented May 21, 2020

Interesting semantics.

The current proposal is overly ambitious in "all subclassing machinery delenda est". I agree with you that subclass creation in itself is a nice intuition, and that without continued support for it, given libraries like Buffer, the compat risk is an order of magnitude more.

Fortunately the test for any of these ideas is to go ahead and try to clean up a single built-in method and see how much complexity in existing engines it actually gets rid of. Could @jandem give some data for SpiderMonkey? I'll do a similar investigation in V8.

@syg
Copy link
Collaborator

syg commented May 21, 2020

An alternative to this proposal is to still drop @@species and only use this.constructor as new.target in instance methods. That's only one Get, but unfortunately still requires a protector in V8 for modifying .constructor on instances of built-ins.

On the compat side, I've been told Microsoft tried shipping subclassing by delegating only to .constructor in ES6 days and found that it was not compatible, and that's how we got @@species. Can anyone from MS dig anything up? @bterlson?

All in all I'd still prefer to get rid of support for Type II subclassing entirely.

@syg
Copy link
Collaborator

syg commented May 21, 2020

I looked a bit in V8 for the code to remove for both @littledan's original proposal of using constructor[@@species] as new.target and my alternative suggestion of only using constructor.

Both don't really completely eliminate code complexity and slow paths, as you might expect, but do seem on paper to increase security guarantees by not having arbitrary subclass constructor run, as Dan said. They do removal some logic, but it is unclear to me at this point if that limited removal is worth the trouble to change from the current behavior.

@littledan
Copy link
Member Author

The main thing is, if you call Array.prototype.map.call(not an array subclass instance), it should instantiate an Array, not this.constructor (think a NodeList, or Array with null prototype, or an object literal with indexed properties). This is fixable with this.constructor in other ways.

@littledan
Copy link
Member Author

So, I guess my suggestion here is in between Type I and Type II. Would it be worth giving it some treatment in the README?

@jandem
Copy link

jandem commented May 22, 2020

Fortunately the test for any of these ideas is to go ahead and try to clean up a single built-in method and see how much complexity in existing engines it actually gets rid of. Could @jandem give some data for SpiderMonkey?

The big pieces in SM are, as far as I can tell:

  1. A cache to make @@species lookups fast in C++ builtins (called ArraySpeciesLookup).
  2. The self-hosted ArraySpeciesCreate implementation.
  3. Various C++ intrinsics called by the self-hosted code and potentially JIT-optimized.

Type I would let us remove each of these. The other options would let us simplify code, but we'd still need the basic machinery in some form. It also depends quite a lot on what ArraySpeciesCreate steps 6-10 would look like, for example the Realm stuff in step 6 is pretty unfortunate as well.

@anba, do you have any thoughts on this? (He added the @@species lookup cache to SpiderMonkey.)

@anba
Copy link

anba commented May 22, 2020

The savings will probably depend on the built-in:

  • For Array I don't expect any performance improvements, because we still need to perform the constructor and @@species lookups (resp. use some mechanism to avoid those lookups in C++). It may make it easier to improve the slow-path, because we're now guaranteed to have actual Array instances, but that's not really the goal of this project, is it? :-)

  • For TypedArray or (Shared)ArrayBuffer, that approach will allow to remove some extra code, because for example TypedArraySpeciesCreate no longer needs to validate that result object is indeed a TypedArray with the correct length and without a detached ArrayBuffer. But those checks are relatively cheap when compared to the constructor and @@species lookups, because the relevant intrinsics need to be JIT inlined with or without @@species.

    • It looks like neither V8 nor SpiderMonkey implement TypedArraySpeciesCreate correctly, step 6 seems to be missing in both implementations. Hmm, relatively cheap may be less cheap than expected?! ;-)

@littledan
Copy link
Member Author

OK, sounds like this wouldn't be a big benefit; no need to focus on this idea.

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

No branches or pull requests

4 participants