Bad performance instantiating class with constructor
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox75 | --- | fixed |
People
(Reporter: nickolay8, Assigned: anba)
References
(Blocks 1 open bug)
Details
(Keywords: perf: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:
- Using normal
new ClassConstructor()
call - Using
Object.create()
+ "external" constructor - a regular method, called "initialize" which does the same thing as constructor - 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()
Reporter | ||
Comment 1•5 years ago
|
||
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)
}
Comment 2•5 years ago
|
||
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!
Updated•5 years ago
|
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Comment 4•5 years ago
|
||
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
Updated•5 years ago
|
Reporter | ||
Comment 5•5 years ago
|
||
The test case is in my report itself? It is also available online on jsperf? The developers should be able to reproduce it easily.
Updated•5 years ago
|
Reporter | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
Fixing bug 1606868, bug 1378189, and bug 1605143 should get the performance for class constructors on a par with the other two instantiation types.
Comment 8•5 years ago
|
||
Steven, considering comment 6, would you please confirm it or assign someone who has the necessary technical knowledge to do it? Thanks.
Comment 9•5 years ago
|
||
I will remove the priority so that it can be re-triaged and prioritized.
Comment 10•5 years ago
|
||
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.
Comment 11•5 years ago
|
||
Thanks to André:
$ dist/bin/js /tmp/bench.js
Time for constructor: 5427
Time for Object.create: 4178
Time for Raw object: 5519
Reporter | ||
Comment 12•5 years ago
|
||
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().
Assignee | ||
Comment 13•5 years ago
|
||
I took another look at the benchmark and I think the following issues would need to be addressed to further optimise class constructors:
- We currently don't use
MCreateThisWithTemplate
, becauseMCreateThisWithTemplate
requires the callee to matchnew.target
. SeeIonBuilder::createThis()
. Fixing this will require some work, because we somehow would need to storenew.target
in the IC stubs, so we can retrieve the correct template object fromBaselineInspector::getTemplateObject()
. - We also don't inline functions when the callee doesn't match
new.target
. This is more easily fixable, at least for knownnew.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:
- The benchmark creates classes outside of a "run-once" context, so when we clone the function in
CloneFunctionObjectIfNotSingleton
, we end up inCloneFunctionReuseScript
, because we typically don't create singletons outside of "run-once" contexts. AndCloneFunctionReuseScript
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. AddingJSFunction::setTypeForScriptedFunction()
toCloneFunctionReuseScript
should fix this issue. - Another "run-once" and hence no singleton object issue is present here. Using a similar approach as in
IonBuilder::getSingleCallTarget()
and usingTemporaryTypeSet::maybeSingleObject()
instead ofTemporaryTypeSet::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.
Reporter | ||
Comment 14•5 years ago
|
||
three times faster in local tests
Sounds very promising!
Assignee | ||
Comment 15•5 years ago
|
||
Allow any single object instead of just singletons to make it possible to
optimise JSOp::SuperFun
outside of "run-once" contexts.
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
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
Assignee | ||
Comment 17•5 years ago
|
||
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
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/71221dd672dc
https://hg.mozilla.org/mozilla-central/rev/9e219b831171
https://hg.mozilla.org/mozilla-central/rev/3f3a19cb49a9
Reporter | ||
Comment 20•5 years ago
|
||
I'm very glad to see it fixed, thanks a lot! So how the initial benchmark now looks like?
Reporter | ||
Comment 21•5 years ago
|
||
Reporter | ||
Comment 22•5 years ago
|
||
Well done, constructor is now the fastest!
Assignee | ||
Comment 23•5 years ago
|
||
Cool, thanks for testing! :-)
Comment 24•5 years ago
|
||
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...
Updated•3 years ago
|
Description
•