Rethink about ClassSpec delegation based on current structure.

RESOLVED FIXED in Firefox 53



2 years ago
2 years ago


(Reporter: arai, Assigned: arai)



Firefox Tracking Flags

(firefox53 fixed)



(1 attachment)



2 years ago
At the point of ClassSpec delegation was introduced (bug 861219), ClassSpec was embedded into Class,
and I think that was one of the main reason why delegation was introduced.

now ClassSpec is separated from Class (bug 1260984), and we can just point other ClassSpec, perhaps a ClassSpec from multiple Class'es.
so, in some case, we could remove delegation, depending on `flags` member.

Comment 1

2 years ago
If ClassSpec is delegated the following method just returns delegated ClassSpec's one:
  * createConstructorHook (for createConstructor_)
  * createPrototypeHook (for createPrototype_)
  * constructorFunctions (for constructorFunctions_)
  * constructorProperties (for constructorProperties_)
  * prototypeFunctions (for prototypeFunctions_)
  * prototypeProperties (for prototypeProperties_)
  * finishInitHook (for finishInit_)

and the following methods are used only internally:
  * defined
    always true if delegated() is true,
    also true for all existing delegated ClassSpecs
      * PromiseObjectClassSpec
      * DateObjectClassSpec
      * ArrayBufferObjectClassSpec
      * TypedArrayObjectClassSpecs
  * delegated
  * delegatedClassSpec

The only difference are the following 2 methods, that behaves differently depending on |flags| field
  * inheritanceProtoKey
    depends |flags & ProtoKeyMask|
  * shouldDefineConstructor
    depends |flags & DontDefineConstructor|

If above flags are same between delegation source and target, we can just point delegated ClassSpec instead of using IsDelegated.

Here's current consumers:
  * Promise
      source:       PromiseObjectProtoClassSpec
      source.flags: ClassSpec::IsDelegated
      target:       PromiseObjectClassSpec
      target.flags: 0

  * Date
      source:       DateObjectProtoClassSpec
      source.flags: ClassSpec::IsDelegated
      target:       DateObjectClassSpec
      target.flags: 0

  * ArrayBuffer
      source:       ArrayBufferObjectProtoClassSpec
      source.flags: ClassSpec::IsDelegated
      target:       ArrayBufferObjectClassSpec
      target.flags: 0

  * SharedArrayBuffer
      source:       SharedArrayBufferObjectProtoClassSpec
      source.flags: ClassSpec::IsDelegated
      target:       ArrayBufferObjectClassSpec
      target.flags: 0

  * %TypedArray%
      source:       TypedArrayObjectProtoClassSpecs
      source.flags: JSProto_TypedArray | ClassSpec::IsDelegated
      target:       TypedArrayObjectClassSpecs
      target.flags: JSProto_TypedArray

|flags & ProtoKeyMask| and |flags & DontDefineConstructor| are same in all cases, and apparently we don't need delegation for current usage.

Comment 2

2 years ago
I forgot 2 new consumers

  * Map
      source:       MapObjectProtoClassSpec
      source.flags: ClassSpec::IsDelegated
      target:       MapObject::classSpec_
      target.flags: 0

  * Set
      source:       SetObjectProtoClassSpec
      source.flags: ClassSpec::IsDelegated
      target:       SetObject::classSpec_
      target.flags: 0

so they're also okay.

Comment 3

2 years ago
try run that removes delegation
Assignee: nobody → arai.unmht

Comment 4

2 years ago
Created attachment 8812645 [details] [diff] [review]
Remove ClassSpec delegation and point target ClassSpec directly from prototype class.

Partially backed out bug 861219 patch that introduces delegation,
so now there's no delegation.

and also changed the reference to ClassSpec with ClassSpec::IsDelegated to the reference to delegation target.
and moved prototype class to class static member to avoid forward declaration in cpp files.
Attachment #8812645 - Flags: review?(jdemooij)
Comment on attachment 8812645 [details] [diff] [review]
Remove ClassSpec delegation and point target ClassSpec directly from prototype class.

Review of attachment 8812645 [details] [diff] [review]:

This is a great refactoring and it really simplifies things. Nice work!
Attachment #8812645 - Flags: review?(jdemooij) → review+

Comment 6

2 years ago
Bug 1318815 - Remove ClassSpec delegation and point target ClassSpec directly from prototype class. r=jandem

Comment 7

2 years ago
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.