Less optimization when adding a single unused variable
Reported by
david.ca...@gmail.com,
Sep 7
|
|||||||
Issue descriptionUserAgent: 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:
,
Sep 7
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.
,
Sep 9
,
Sep 10
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.
,
Sep 19
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.
,
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?
,
Sep 19
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?
,
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.
,
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.
,
Sep 20
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.
,
Sep 20
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_).
,
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 |
|||||||
Comment 1 by kbr@chromium.org
, Sep 7