Cleanup JSProtoKeys and object creation for typed arrays

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
I've never understood the convoluted object creation path that typed array related classes go through, so here's my attempt at stripping it down to something the actually makes sense to me, so that I can pester bhackett to tell me where I'm going wrong.

See also bug 761121.
(Assignee)

Comment 1

6 years ago
Created attachment 630074 [details] [diff] [review]
Make typed array related classes use HAS_CACHED_PROTO for the instance class only, and create objects with the final class immediately

One remaining weirdness -- this still creates a whole new EmptyShape, apparently only to set the BaseShape's NOT_EXTENSIBLE flag. Is there a better way of doing that?

I'm also unclear on whether this does the right thing with respect to identifying separate allocation sites for the purpose of type inference.
Attachment #630074 - Flags: review?(bhackett1024)
Bug 761121 (minimal fix for this), which sfink wants to dupe to this bug, blocks bug 758415. That bug is very time sensitive due to crashes and addon compat, and it needs to land on both m-c and aurora as soon as possible.

My hope is that bhackett will just say that the patch here is definitely correct and can land immediately on both branches. But in the event of an rminus, bhackett, can you comment on the best way forward to get bug 758415 landed?
Comment on attachment 630074 [details] [diff] [review]
Make typed array related classes use HAS_CACHED_PROTO for the instance class only, and create objects with the final class immediately

Review of attachment 630074 [details] [diff] [review]:
-----------------------------------------------------------------

The changes that remove calls to TypeCallerInitObject aren't good, as this will cause those objects to not be statically known to be typed arrays and will nuke performance on scripts which use typed arrays.

Changes to which class is used for ArrayBufferObject and DataViewObject aren't necessary, as these classes already have the same JSCLASS_HAS_CACHED_PROTO value.  Thus, when figuring out the cached proto for such classes, it's fine to use either the data or the proto class as the same result will be achieved.

I think that doing the same for the typed array classes is the best solution here, i.e. add JSCLASS_HAS_CACHED_PROTO(JSProto_##_typedArray) to IMPL_TYPED_ARRAY_FAST_CLASS.  That seems a more robust fix than the one in bug 761121.  r=me for that patch if you or bholley wants to push it (push to try first though!).

More generally, I think the fast + slow class distinction is a (longstanding) design trainwreck and there should just be a single class with the flexibility necessary to represent either instances or the proto.
Attachment #630074 - Flags: review?(bhackett1024) → review-
(Assignee)

Comment 4

6 years ago
Ok, bholley's time-sensitive thing is done, so now I really want to understand this because I've run up against it multiple times now, and I hate cargo-culting code.

(In reply to Brian Hackett (:bhackett) from comment #3)
> Comment on attachment 630074 [details] [diff] [review]
> Make typed array related classes use HAS_CACHED_PROTO for the instance class
> only, and create objects with the final class immediately
> 
> Review of attachment 630074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes that remove calls to TypeCallerInitObject aren't good, as this
> will cause those objects to not be statically known to be typed arrays and
> will nuke performance on scripts which use typed arrays.

That was very much what I wanted to know, but I still don't follow the details. Is there documentation I can read? Specific questions:

Why is GetTypeCallerInitObject necessary to statically know that something is a typed array? Why doesn't NewBuiltinClassInstance do that automatically?

> Changes to which class is used for ArrayBufferObject and DataViewObject
> aren't necessary, as these classes already have the same
> JSCLASS_HAS_CACHED_PROTO value.  Thus, when figuring out the cached proto
> for such classes, it's fine to use either the data or the proto class as the
> same result will be achieved.

But why would you want to? I don't understand why you'd ever want to use the proto class, especially since enabling that means that NewBuiltinClassInstance(&protoClass) seems like it must construct a strange prototype object whose prototype is an object of the same class. In practice, the code that does this then morphs that object into a different class, which strikes me as totally cryptic and unnecessary. Which means that it must be necessary for some reason I don't understand. What is it?

> I think that doing the same for the typed array classes is the best solution
> here, i.e. add JSCLASS_HAS_CACHED_PROTO(JSProto_##_typedArray) to
> IMPL_TYPED_ARRAY_FAST_CLASS.  That seems a more robust fix than the one in
> bug 761121.  r=me for that patch if you or bholley wants to push it (push to
> try first though!).

Works for me, though I'm still trying to understand why that's the preferred path.

> More generally, I think the fast + slow class distinction is a
> (longstanding) design trainwreck and there should just be a single class
> with the flexibility necessary to represent either instances or the proto.

Why? Waldo seems to share the same opinion. Clearly, the WebIDL guys don't, since they pretty much mandate it in the spec. (I even had all of the typed array stuff implemented that way once upon a time, and rewrote it because I think bz complained about the discrepancy with WebIDL.) I don't see the reasons to prefer either one over the other.

The only thing about it I really dislike is calling them "fast" vs "slow" instead of "instance" vs "proto", but I've already eliminated the fast/slow terminology from the typed array code.

Finally, what about the last remaining change in this patch? Namely, calling NewBuiltinClassInstance with the instance Class* instead of using the proto one and morphing it later? (And the followup question about a cleaner-looking way of setting the NOT_EXTENSIBLE flag on the BaseShape.)
(In reply to Steve Fink [:sfink] from comment #4)
> Why is GetTypeCallerInitObject necessary to statically know that something
> is a typed array? Why doesn't NewBuiltinClassInstance do that automatically?

NewBuiltinClassInstance will use the generic 'new' type of the prototype, which will not be known statically to be a typed array or have other object-specific metadata attached.  This is because it is possible to create objects that don't have these properties and also share the prototype, such as 'Object.create(Uint8Array.prototype)'.  So static information about the kind of object a type object represents is attached in other ways, and the main way for arrays / type objects is to tie the type object to a specific allocation site + proto key combo.

> > Changes to which class is used for ArrayBufferObject and DataViewObject
> > aren't necessary, as these classes already have the same
> > JSCLASS_HAS_CACHED_PROTO value.  Thus, when figuring out the cached proto
> > for such classes, it's fine to use either the data or the proto class as the
> > same result will be achieved.
> 
> But why would you want to? I don't understand why you'd ever want to use the
> proto class, especially since enabling that means that
> NewBuiltinClassInstance(&protoClass) seems like it must construct a strange
> prototype object whose prototype is an object of the same class. In
> practice, the code that does this then morphs that object into a different
> class, which strikes me as totally cryptic and unnecessary. Which means that
> it must be necessary for some reason I don't understand. What is it?

I just suggested being able to use either the data or the proto class as a way to avoid the foot gun whose usage bholley had to fix.  Requiring the data class to be specifically used would be OK as long as there were strong asserts preventing the prototype class from being used for cached lookups.

> Why? Waldo seems to share the same opinion. Clearly, the WebIDL guys don't,
> since they pretty much mandate it in the spec. (I even had all of the typed
> array stuff implemented that way once upon a time, and rewrote it because I
> think bz complained about the discrepancy with WebIDL.) I don't see the
> reasons to prefer either one over the other.

I don't know enough about the spec to say for sure, I'm mostly just reacting to all the mess that's in the typed array code.  What are the semantic issues that prevented you from being able to combine the typed array stuff into a single set of classes?

> Finally, what about the last remaining change in this patch? Namely, calling
> NewBuiltinClassInstance with the instance Class* instead of using the proto
> one and morphing it later? (And the followup question about a
> cleaner-looking way of setting the NOT_EXTENSIBLE flag on the BaseShape.)

That's good, and definitely a needed improvement as after the rm-slowarray stuff we need to be in a place where an object's class is immutable, so that it can move to the TypeObject.  I think obj->preventExtensions could be used and would be cleaner.
Whiteboard: [js:t]
(Assignee)

Comment 6

6 years ago
(In reply to Brian Hackett (:bhackett) from comment #5)
> (In reply to Steve Fink [:sfink] from comment #4)
> > Why? Waldo seems to share the same opinion. Clearly, the WebIDL guys don't,
> > since they pretty much mandate it in the spec. (I even had all of the typed
> > array stuff implemented that way once upon a time, and rewrote it because I
> > think bz complained about the discrepancy with WebIDL.) I don't see the
> > reasons to prefer either one over the other.
> 
> I don't know enough about the spec to say for sure, I'm mostly just reacting
> to all the mess that's in the typed array code.  What are the semantic
> issues that prevented you from being able to combine the typed array stuff
> into a single set of classes?

Sorry, I never answered this. WebIDL says that accessing properties on an interface prototype object should throw a TypeError. More directly, the class name is supposed to be eg "Int32ArrayPrototype" not "Int32Array". Perhaps there are ways to fake both of those using the same class, and whatever else is lurking in WebIDL, but you'd be fighting against the spec. (It is possible that the ES spec may take over typed arrays, in which case all bets are off.)
(Assignee)

Comment 7

6 years ago
I think I've now landed anything landable from this bug.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.