Closed Bug 1557765 Opened 2 years ago Closed 9 months ago

Bad performance instantiating class with constructor

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

66 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: nickolay8, Assigned: anba)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p3:responsiveness])

Attachments

(5 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

I'm benchmarking various ways of class instantiation. The class consists from 6 mixins, every mixin initializes 3 properties in the constructor: 'a1', 'b1', 'c1' for Mixin1 and so forth.

Then I measure the time of creation of 10000 instances, in 3 different ways:

  1. Using normal new ClassConstructor() call
  2. Using Object.create() + "external" constructor - a regular method, called "initialize" which does the same thing as constructor
  3. Using plain object and plain initializing functions

The benchmark is here: https://jsperf.com/object-instantiation-2

Actual results:

The normal instantiation path using class constructor is the slowest - twice (!) slower than other "tricky" paths.

Note, that Chrome behaves as expected.

Expected results:

Expected is that using class constructor should be the fastest, or, on par with Object.create()

Pasting full code also here:

const Mixin1 = (base) =>
class extends base {
    constructor () {
        super()

        this.a1 = []

        this.b1 = 11

        this.c1 = new Set()
    }


    initialize () {
        super.initialize()

        this.a1 = []

        this.b1 = 11

        this.c1 = new Set()
    }
}


const initialize1 = function () {
    this.a1 = []

    this.b1 = 11

    this.c1 = new Set()
}



const Mixin2 = (base) =>
class extends base {
    constructor () {
        super()

        this.a2 = []

        this.b2 = 11

        this.c2 = new Map()
    }


    initialize () {
        super.initialize()

        this.a2 = []

        this.b2 = 11

        this.c2 = new Map()
    }
}

const initialize2 = function () {
    this.a2 = []

    this.b2 = 11

    this.c2 = new Map()
}



const Mixin3 = (base) =>
class extends base {
    constructor () {
        super()

        this.a3 = {}

        this.b3 = 'asda'

        this.c3 = false
    }

    initialize () {
        super.initialize()

        this.a3 = {}

        this.b3 = 'asda'

        this.c3 = false
    }
}

const initialize3 = function () {
    this.a3 = {}

    this.b3 = 'asda'

    this.c3 = false
}



const Mixin4 = (base) =>
class extends base {
    constructor () {
        super()

        this.a4 = {}

        this.b4 = 'asda'

        this.c4 = false
    }

    initialize () {
        super.initialize()

        this.a4 = {}

        this.b4 = 'asda'

        this.c4 = false
    }
}


const initialize4 = function () {
    this.a4 = {}

    this.b4 = 'asda'

    this.c4 = false
}



const Mixin5 = (base) =>
class extends base {
    constructor () {
        super()

        this.a5 = {}

        this.b5 = 'asda'

        this.c5 = false
    }

    initialize () {
        super.initialize()

        this.a5 = {}

        this.b5 = 'asda'

        this.c5 = false
    }
}

const initialize5 = function () {
    this.a5 = {}

    this.b5 = 'asda'

    this.c5 = false
}


const Mixin6 = (base) =>
class extends base {
    constructor () {
        super()

        this.a6 = {}

        this.b6 = 'asda'

        this.c6 = false
    }

    initialize () {
        super.initialize()

        this.a6 = {}

        this.b6 = 'asda'

        this.c6 = false
    }
}

const initialize6 = function () {
    this.a6 = {}

    this.b6 = 'asda'

    this.c6 = false
}



class Base {
    initialize () {
    }
}

const cls = Mixin6(Mixin5(Mixin4(Mixin3(Mixin2(Mixin1(Base))))))

const count         = 10000
const instances     = []


// ;[ 'a1', 'b1', 'c1', 'a2', 'b2', 'c2', 'a3', 'b3', 'c3' ].forEach(prop => cls.prototype[ prop ] = null)


for (let i = 0; i < count; i++) instances.push(new cls())


for (let i = 0; i < count; i++) {
    const instance  = Object.create(cls.prototype)

    instance.initialize()

    instances.push(instance)
}


for (let i = 0; i < count; i++) {
    const instance  = {}

    initialize1.call(instance)
    initialize2.call(instance)
    initialize3.call(instance)
    initialize4.call(instance)
    initialize5.call(instance)
    initialize6.call(instance)

    instances.push(instance)
}

This bug has a pretty high technical level and I do not have the skill to understand it or confirm it.
I will set its component as (Core) Performance and hope that a dev can have a more appropriate opinion on it.

@Nickolay Platonov: Can you provide me with a test case if it needs to be confirmed?
@DEVs/triage owner: If the component is not correct, please set a more appropriate one rather than leaving it untriaged or on the General one.

Also Ni me if certain manual testing is needed along with information on how to do it. Thank you!

Component: Untriaged → Performance
Product: Firefox → Core
Flags: needinfo?(nickolay8)
Component: Performance → JavaScript Engine
Whiteboard: [qf]
Attached file bench.js

Wrapped the benchmark from comment 1 with some timings. Easy to reproduce from the shell:
$ dist/bin/js bench.js
Time for constructor: 9858
Time for Object.create: 4878
Time for Raw object: 4395

Whiteboard: [qf] → [qf:p3:responsiveness]

The test case is in my report itself? It is also available online on jsperf? The developers should be able to reproduce it easily.

Flags: needinfo?(nickolay8)
Component: JavaScript Engine → JavaScript Engine: JIT
Priority: -- → P3

Any updates on this issue? It is still in "UNCONFIRMED" state.

Isn't it pretty bad, that most natural way of class instantiation is twice slower than some "magickery" one? Fixing it should speed up all the code out there.

Fixing bug 1606868, bug 1378189, and bug 1605143 should get the performance for class constructors on a par with the other two instantiation types.

Depends on: 1606868, 1378189, 1605143

Steven, considering comment 6, would you please confirm it or assign someone who has the necessary technical knowledge to do it? Thanks.

Flags: needinfo?(sdetar)

I will remove the priority so that it can be re-triaged and prioritized.

Flags: needinfo?(sdetar)
Priority: P3 → --

Knowing that this work is blocked by bug 1378189, and that it is unlikely to be implemented before Bug 1613592.
I will set it P3.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3

Thanks to André:

$ dist/bin/js /tmp/bench.js 
Time for constructor: 5427
Time for Object.create: 4178
Time for Raw object: 5519
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED

Great progress. However, instantiation with constructor is still 1.3x times slower than with Object.create(). I don't think this ticket is fixed - class constructor should be, at the very least, on par with Object.create().

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

I took another look at the benchmark and I think the following issues would need to be addressed to further optimise class constructors:

  1. We currently don't use MCreateThisWithTemplate, because MCreateThisWithTemplate requires the callee to match new.target. See IonBuilder::createThis(). Fixing this will require some work, because we somehow would need to store new.target in the IC stubs, so we can retrieve the correct template object from BaselineInspector::getTemplateObject().
  2. We also don't inline functions when the callee doesn't match new.target. This is more easily fixable, at least for known new.target values, because we could reuse the same approach from here and apply it here.

The last item won't actually help for the benchmark, because of two further issues:

  1. The benchmark creates classes outside of a "run-once" context, so when we clone the function in CloneFunctionObjectIfNotSingleton, we end up in CloneFunctionReuseScript, because we typically don't create singletons outside of "run-once" contexts. And CloneFunctionReuseScript doesn't preserve the group, because the prototypes don't match for classes. And not preserving the group means we don't have a function object ready here. Adding JSFunction::setTypeForScriptedFunction() to CloneFunctionReuseScript should fix this issue.
  2. Another "run-once" and hence no singleton object issue is present here. Using a similar approach as in IonBuilder::getSingleCallTarget() and using TemporaryTypeSet::maybeSingleObject() instead of TemporaryTypeSet::maybeSingleton() should help here.

Addressing points 2-4 made the raw (*) class constructor case about three times faster in local tests. And when I cheated (**) a bit to workaround point 1, I could verify that it's possible to optimise away the class constructors calls.

(*) "raw" insofar that I've modified the test case to not create any properties on the objects and also removed the Array.prototype.push calls. Basically removing everything where we'd end up measuring things apart from the actual class constructor code.
(**) I've changed the base class from class Base {} to class Base extends class {} { constructor() { return {}; } } to avoid emitting MCreateThisWithProto.


TLDR: The remaining difference is not being able allocate the this objects in assembly code and not aggressively inlining the nested derived class constructor calls.

three times faster in local tests

Sounds very promising!

Allow any single object instead of just singletons to make it possible to
optimise JSOp::SuperFun outside of "run-once" contexts.

Assignee: nobody → andrebargull

Allow to inline when callee and new.target don't match, as long as the callee
is a derived class constructor, because for derived class constructors we don't
create the this-object at the call-site. This allows to inline through a chain
of derived class constructors.

Depends on D64970

Allow to inline when callee and new.target don't match, if new.target is
guaranteed to have a non-configurable "prototype" property. This change allows
to inline to a base class constructor when called from a derived class
constructor.

Depends on D64971

Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71221dd672dc
Part 1: Relax jsop_superfun to allow any single object. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9e219b831171
Part 2: Inline through derived class constructors. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3f3a19cb49a9
Part 3: Inline base class constructor when called from derived class constructors. r=jandem
Status: REOPENED → RESOLVED
Closed: 10 months ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

I'm very glad to see it fixed, thanks a lot! So how the initial benchmark now looks like?

Attached image new benchmark

Well done, constructor is now the fastest!

Cool, thanks for testing! :-)

I am pretty sure that this improvement had the nasty side effect of breaking my JavaScript app. Are you absolutely sure that this change is a safe change? I don't understand these type of optimizations (yet), but a bisection for a bug we experienced lead me to this issue and lead me to believe that this actually broke the JavaScript engine in FF for our (and of course potentially others) apps:

Please see here for details and if you anyone of you who understands the problem and the patch has any doubt that this optimization is a safe thing to do, I highly recommend to roll it back ASAP :-( Sorry...

https://phabricator.services.mozilla.com/D58804#2259337

You need to log in before you can comment on or make changes to this bug.