New issue
Advanced search Search tips

Issue 881977 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

Less optimization when adding a single unused variable

Reported by david.ca...@gmail.com, 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 http://www.babylonjs.com/demos/fatobjects/slim/
2. Check the FPS
3. Open http://www.babylonjs.com/demos/fatobjects/fat/
4. Check the FPS

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

What went wrong?
The fat version (http://www.babylonjs.com/demos/fatobjects/fat/) 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:
 
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.

Cc: kbr@chromium.org
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.

Labels: Needs-Triage-M69
Components: -Blink>JavaScript>Compiler Blink>JavaScript>Runtime
Owner: ishell@chromium.org
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.
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.
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? 
So to workaround it, what should we do?

Should we initialize all fields like here https://github.com/BabylonJS/Babylon.js/blob/master/src/babylon.node.ts#L69 (this will add an init in the constructor) ?

So for this line: https://github.com/BabylonJS/Babylon.js/blob/master/src/babylon.node.ts#L45, 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?


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

Cc: verwa...@chromium.org
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_).

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

Thanks again

Sign in to add a comment