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

Unnecessary `Symbol.iterator` call in subclass default constructor #827

Closed
isiahmeadows opened this Issue Feb 26, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@isiahmeadows

isiahmeadows commented Feb 26, 2017

In 14.5.13 Runtime Semantics: ClassDefinitionEvaluation, step 10.a.i, it specifies constructor(...args) { super(...args) } as the default constructor. As per 12.3.6.1 Runtime Semantics: ArgumentListEvaluation, this always incurs an observable read and call of Array.prototype[Symbol.iterator]. That inhibits the performance optimization of just straight delegating, and it also means at least Babel isn't conforming.

I suspect that wasn't intentional?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 26, 2017

Member

I'm reasonably certain it was intentional, and I believe engines are capable of optimizing it when Array.prototype[Symbol.iterator] is unmodified.

Member

ljharb commented Feb 26, 2017

I'm reasonably certain it was intentional, and I believe engines are capable of optimizing it when Array.prototype[Symbol.iterator] is unmodified.

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Feb 26, 2017

@ljharb I meant being able to optimize without the runtime check.

isiahmeadows commented Feb 26, 2017

@ljharb I meant being able to optimize without the runtime check.

@efaust

This comment has been minimized.

Show comment
Hide comment
@efaust

efaust Feb 26, 2017

As the FF engine dev for this very feature, I agree that this was annoying, and I did trip over it in exactly this same way, but I've come around.

In ES6, the most obvious and intuitive way to express that function is the way they do it in the spec:

constructor(...args) {
   super(...args);
}

We self-host ours, and that's exactly as it appears in the FF source.

The whole idea of default constructors is, "we wrote this the same way you would have, and it's there as a convenience so that you don't have to type it."

This is the intuition, and therefore the expectation. Whether it's the easiest for engine devs, it's the easiest for JS devs to understand. A single engine-side runtime check (which JIT authors can move to compile-time, with modern guarding systems) is a small price to pay for the simplicity of that mental model.

efaust commented Feb 26, 2017

As the FF engine dev for this very feature, I agree that this was annoying, and I did trip over it in exactly this same way, but I've come around.

In ES6, the most obvious and intuitive way to express that function is the way they do it in the spec:

constructor(...args) {
   super(...args);
}

We self-host ours, and that's exactly as it appears in the FF source.

The whole idea of default constructors is, "we wrote this the same way you would have, and it's there as a convenience so that you don't have to type it."

This is the intuition, and therefore the expectation. Whether it's the easiest for engine devs, it's the easiest for JS devs to understand. A single engine-side runtime check (which JIT authors can move to compile-time, with modern guarding systems) is a small price to pay for the simplicity of that mental model.

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Feb 26, 2017

Contributor

Whether it's the easiest for engine devs, it's the easiest for JS devs to understand.

True, but this version of the default constructor could be just non-normative suggestive text instead of normative required text. That would help JS devs understand without requiring that potential side-effect or complicating optimizations. At least, that's how I interpret the point of the OP.

Contributor

getify commented Feb 26, 2017

Whether it's the easiest for engine devs, it's the easiest for JS devs to understand.

True, but this version of the default constructor could be just non-normative suggestive text instead of normative required text. That would help JS devs understand without requiring that potential side-effect or complicating optimizations. At least, that's how I interpret the point of the OP.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 26, 2017

Member

It could, but that would allow one to observe whether a class had an explicit constructor or not, which would make refactoring it to have one or to not observable as well. Requiring identical semantics for both cases avoids that.

Member

ljharb commented Feb 26, 2017

It could, but that would allow one to observe whether a class had an explicit constructor or not, which would make refactoring it to have one or to not observable as well. Requiring identical semantics for both cases avoids that.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Feb 26, 2017

Member

When I originally wrote the spec. for ClassDefinitionEvaluation I don't believe I considered the possibility that Array.prototype[Symbol.iterator] could have been modified to some non-standard value. If I had thought about it, I might have specified the default constructor behavior in a different manner that wouldn't have been sensitive to such a change.

But, I didn't and as has been pointed out in this thread optimizing engines should be able to deal with this. It doesn't seem like something that is worth changing in wither the spec. or implementations unless somebody can show a significant real-world impact of the current spec.

Member

allenwb commented Feb 26, 2017

When I originally wrote the spec. for ClassDefinitionEvaluation I don't believe I considered the possibility that Array.prototype[Symbol.iterator] could have been modified to some non-standard value. If I had thought about it, I might have specified the default constructor behavior in a different manner that wouldn't have been sensitive to such a change.

But, I didn't and as has been pointed out in this thread optimizing engines should be able to deal with this. It doesn't seem like something that is worth changing in wither the spec. or implementations unless somebody can show a significant real-world impact of the current spec.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Apr 17, 2017

Member

V8 used to desugar to arguments and @gsathya changed it to desugaring to rest/spread. It felt sort of silly at the time, but at this point, thanks to optimizations by @petermarshall, I believe it's just about as efficient as the old form (though V8 has a quick runtime check IIRC).

Member

littledan commented Apr 17, 2017

V8 used to desugar to arguments and @gsathya changed it to desugaring to rest/spread. It felt sort of silly at the time, but at this point, thanks to optimizations by @petermarshall, I believe it's just about as efficient as the old form (though V8 has a quick runtime check IIRC).

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Apr 17, 2017

Since it appears my performance concerns are invalid and the bug likely just going to stay, I'm going to go ahead and close this.

isiahmeadows commented Apr 17, 2017

Since it appears my performance concerns are invalid and the bug likely just going to stay, I'm going to go ahead and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment