`Object.defineProperty` in `THREE.Vector3` constructor causing a significant performance penalty

[pjanik]

Describe the bug

I've noticed a significant performance drop in my application when I upgraded the THREE.JS version. Initially from 0.102.0 to 0.125.2, but I've narrowed it down to a change from 0.119.1 to 0.120.0. A single model step in my app (simulation) increased from ~10ms to ~50ms.

I've used JS profiler and noticed that Vector3.clone() is at the top now:

I'm using THREE.Vector3 in the simulation part of the app, and since many vector operations are implemented in place, .clone() calls are very common. When I downgrade version to 0.119.1, the clone operation isn't there anymore and the model step calculates 4 times faster.

I did a little investigation and replacing Object.defineProperty( this, 'isVector3', { value: true } ); in THREE.Vector3 constructor with this.isVector3 = true; fixes the issue.

To Reproduce, Live example

• r125, slow clone: https://jsfiddle.net/9hrmuLxw/ (open console to see the result)
• r120, slow clone: https://jsfiddle.net/nb0uw1ay/ (open console to see the result)
• r119, fast clone: https://jsfiddle.net/p2bu1qte/ (open console to see the result)

On my machine, the difference is 5ms (r119) vs 45ms (r120, r125).

I don't think it'd be useful here, but the app I'm working on:
https://tectonic-explorer.concord.org/?preset=subduction

Expected behavior

No .clone performance drop.

Screenshots

THREE.JS version 0.119.1 vs 0.120.0:

Platform:

• Device: Desktop
• OS: MacOS
• Browser: Chrome v88
• Three.js version: r120.0 and newer, r119 is the last one that works fine

[mrdoob]

@marcofugaro @DefinitelyMaybe I think we should just do this.isVector3 = true;.

[marcofugaro]

@mrdoob hmm yep, I agree.

@pjanik thanks for pinpointing the issue.

[pjanik]

I guess that's obvious, but I can see more Object.defineProperty usage in the three.js codebase. Vector3 is specific to my app. I guess results would be the same if I did a similar benchmark with Vector2 or Matrix3, etc. Also, I've tried Chrome Canary (v90) and no difference there.

[mrdoob]

I think I'll just make a script and fix all.

[mrdoob]

PR: #21285

[WestLangley]

I'm using THREE.Vector3 in the simulation part of the app, and since many vector operations are implemented in place, .clone() calls are very common.

IMHO, you should not be using .clone() in that manner as it causes unnecessary instantiation of objects and garbage-collection. Create working instances and reuse them -- or use a closure.

[mrdoob]

@WestLangley that's a good point, but this is definitely a regression.

[WestLangley]

User's have been advised -- certainly by me -- not to call clone() in a tight loop. If that is what the OP is doing, perhaps he can fix his problem at the app level.

[pjanik]

FYI, my jstfiddle example is just the simplest test case I could imagine. It doesn't have much in common with the real app. I'm not calling .clone() 100k times in one loop in the real world. 😉

My .clone() calls are spread all over the codebase, including the simulation engine where the problem is. It's thousands of lines, different classes, methods, and getters. It's not a single 10-100-200 lines block I could easily tweak that way. As usual, it's performance vs readability and maintainability. I don't think "caching" vectors would be even possible in my case. I'd rather convert my sim code to use a different math library if I really had to.

Thanks for a quick reply and fix!

[WestLangley]

My .clone() calls are spread all over the codebase... It's thousands of lines, different classes, methods, and getters.

@pjanik Right. That is what I assumed.

If you are calling clone() often enough per animation frame to incur a measurable performance hit, then you are misusing the method.

The three.js math classes do not use the method at all.

Just a friendly tip. :-)