Closed Bug 1318815 Opened 6 years ago Closed 6 years ago

Rethink about ClassSpec delegation based on current structure.


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: arai, Assigned: arai)




(1 file)

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.
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.
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.
try run that removes delegation
Assignee: nobody → arai.unmht
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+
Bug 1318815 - Remove ClassSpec delegation and point target ClassSpec directly from prototype class. r=jandem
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.