Rethink about ClassSpec delegation based on current structure.

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months 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.
(Assignee)

Comment 1

11 months 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.
(Assignee)

Comment 2

11 months 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.
(Assignee)

Comment 3

11 months ago
try run that removes delegation
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f7854959afe4b049d61a24c43abc9c478d7735
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
(Assignee)

Comment 4

11 months 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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f7854959afe4b049d61a24c43abc9c478d7735
Attachment #8812645 - Flags: review?(jdemooij)

Comment 5

11 months ago
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+
(Assignee)

Comment 6

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/177b7924440c450525d1127c9c2f1d3d642e4a2c
Bug 1318815 - Remove ClassSpec delegation and point target ClassSpec directly from prototype class. r=jandem

Comment 7

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/177b7924440c
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months 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.