New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Assigned
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 2
Type: Bug

Sign in to add a comment

Issue 881977: Less optimization when adding a single unused variable

Reported by, Sep 7

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.81 Safari/537.36

Steps to reproduce the problem:
1. Open
2. Check the FPS
3. Open
4. Check the FPS

What is the expected behavior?
Both should have the same performance

What went wrong?
The fat version ( is slower.

Only ifference is that fat version has ONE additional property (named I_AM_NOW_FAT) added to the AbstractMesh class (line #19666 in babylon.max.js) 

This variable is not used.

Did this work before? N/A 

Chrome version: 69.0.3497.81  Channel: stable
OS Version: 10.0
Flash Version:

Comment 1 by, Sep 7

Components: -Blink Blink>JavaScript>Compiler
V8 compiler team: could you please triage this? It's surprising that there would be such a performance difference from adding a single field to a "class" (really a "function" in its "constructor"); all of the other properties of this class are added in the same way.

Comment 2 by, Sep 7

Labels: OS-Mac
For what it's worth, I can see the performance difference on 70.0.3534.0 (Official Build) canary (64-bit) on macOS. Slim version is ~50 FPS, fat version is ~30 FPS.

Comment 3 by, Sep 9

Labels: Needs-Triage-M69

Comment 4 by, Sep 10

Components: -Blink>JavaScript>Compiler Blink>JavaScript>Runtime
Status: Assigned (was: Unconfirmed)
Sounds a lot like it's hitting some object property number cliff, maybe it goes to dictionary mode even. Assigning to ishell@ for further investigation.

Comment 5 by, Sep 19

Status: WontFix (was: Assigned)
Yes, exactly, that's our heuristics. Because of I_AM_NOW_FAT property the Mesh.prototype.render() function turns Mesh object into dictionary mode when it adds 129th property _effectiveMaterial.
The workaround could be to ensure that all the fields that may potentially appear later are added in the constructor function using "this.blah = .." assignments.

So, to some extend this is working as intended.

Comment 6 by, Sep 19

Hello, I'm not sure to follow you here because I_AM_NOW_FAT is already initialized in the constructor. So based on your workaround it should not trigger the dictionary mode right?

Comment 7 by, Sep 19

So to workaround it, what should we do?

Should we initialize all fields like here (this will add an init in the constructor) ?

So for this line:, if we initialize it in the constructor will it reduce the number of properties that count for your threshold ?

Saying that differently: What is precisely taken in account to decide when to switch to dictionary mode?

Comment 8 by, Sep 19

To be short: V8 has "soft" limit for turning objects into dictionary mode (the one you observe) and there is also a "hard" limit. In order to be able to go beyond the "soft" limit the properties must "predefined" in the constructor function in the form of this.blah = ...; like here:
  function A() {
    this.a0 = 0;
    this.a1 = undefined;
    this.I_AM_NOW_FAT = false;
    this._effectiveMaterial = undefined;
    this.a1023 = "foo";
Given that in Babylon you add properties through "_this" the heuristics I'm talking about here will not trigger anyway.

I think the other way forward would be to avoid objects with too many properties on hot paths of your code. You may consider moving some of the fields into sub-objects. Or use ES6 classes instead of functions if that's possible.

Comment 9 by, Sep 19

Re #7, yes, but I guess it depends on the result of transpiling. If the resulting code that will reach V8 would be like in /demos/fatobjects/fat/babylon.max.js (_this.I_AM_NOW_FAT = ...;) then it will not help anyway.

Comment 10 by, Sep 20

Status: Started (was: WontFix)
ishell@: we need a better resolution than a quick "WontFix". Please work with this important customer to figure out something that will work for them and other transpiled code. If adjustments to V8's heuristics are needed then let's make them.

Comment 11 by, Sep 20

Status: Assigned (was: Started)
Ok, we've discussed some ideas about new heuristics that should be transpiled-ES6-classes friendly. It makes sense to try them however we can't start immediately and we have to plan this work. 
I'll keep this issue as P2 assigned to me for now.

The temporary V8-friendly workaround would be to
1) define another class AbstractMeshData and move there some rarely used AbstractMesh properties
2) add a AbstractMeshData field into the AbstractMesh and initialize it with an instance in the AbstractMesh constructor

If V8 will never see other values in the data field than the AbstractMeshData instance then the access "mesh.data_.blah_" would be as efficient as possible (just an additional indirection through data_).

Comment 12 by, Sep 20

Thanks a lot! 
We already started moving some data (rarely used) to a storage class;)

Thanks again

Sign in to add a comment