FBXLoader: replace for loops with array builtins and tidy code

[looeee]

Replace for loops with array builtins - forEach, map or reduce, and some other tidying.
Also made the parseConnections method a little easier to read.

[mrdoob]

Similarly to a previous PR. I remember adding these for loops to avoid instantiation of new functions. For loops tend to perform much faster than forEach. The main goal at the time was to speed up parsing and avoid garbage collection.

Have you had a look at Chrome Dev Tools' performance tab to see how these changes affect?

[takahirox]

Have you read Code style?

https://github.com/mrdoob/three.js/wiki/Mr.doob%27s-Code-Style%E2%84%A2#cycles

[looeee]

Yes, I did quite a bit of performance testing as I was making this PR - at first I was making sure to just replace small loops (max 10 - 100 loops), but even testing against large loops (the geometry creation function and the text parser) I didn't see any evidence of slowing down, at least in Chrome.

If you prefer, I can undo the changes to large loops. For smaller loops (let's say, likely to be less than 100 loops), the performance difference is negligible and forEach is much more readable.

I'll do some more testing with and without this PR and report on performance differences.

[takahirox]

I prefer keeping the current for loop because of not only performance but also code consistency among files. I want you to add comment like "// Using forEach here because this loop is always small enough" if you would like to use forEach because the size of loop you edited is very dependent on model and readers can't know the size from the code.

[looeee]

// Using forEach because this loop is always small enough" if you would like to use forEach

That seems redundant to me. I'd rather it be the other way - forEach loops should be the default and for loops should only be used when performance is likely to be an issue.

In any case, I'll test when I have time later today - it's possible that the performance differences between for and forEach have dropped a lot since the code style guide was written, this is a good opportunity to test that at least.

[takahirox]

I'd rather it be the other way - forEach loops should be the default and for loops should only be used when performance is likely to be an issue.

I think you should've first made an issue to discuss if we should make forEach the default loop rather than for before making this PR because it's entire Three.js topic.

[mrdoob]

In any case, I'll test when I have time later today - it's possible that the performance differences between for and forEach have dropped a lot since the code style guide was written, this is a good opportunity to test that at least.

Sounds good! Curious to see the results 👌

[looeee]

OK, here's the results of some initial testing on my laptop.

Models

Tested with three models:

• wafp.bin.fbx ( mixamo model with motion capture anims, binary, compressed, 4088kb)
• wafp.fbx ( same model reexported in ASCII format, 13124kb )
• xsi_man_skinning.fbx (binary, uncompressed, 3004kb)

Methodology:

Testing parsing time wrapping the the parse method inside a console.time in the loader and taking an average of ten runs:

console.time( 'Parsing took: ' );
var scene = self.parse( buffer, resourceDirectory );
console.timeEnd( 'Parsing took:' );

Tested memory use in Chrome using the dev tool performance tab and reloading the page.
Tested memory in Edge using the dev tools memory tab and watching the graph of memory use while reloading the page
No memory tests in Firefox

Browsers:

All files loaded from local file system on Windows 10, caching disabled.

• Chrome 63.0.3239.59 (Official Build) beta (64-bit)
• Firefox 57.0 (64-bit)
• Microsoft Edge 41.17025.1000.0

Firefox updated to the "quantum" version half way through testing so I had to redo those 😕
It is really fast though!

Results:

For reach pair of numbers below, the first is running the dev branch of FBXLoader, and the second with this PR applied.

wafp.bin.fbx (binary, compressed)

Chrome, for, forEach:
Parsing time: 573ms, 514ms
Memory: 82.2mb, 84.9mb

FireFox ,for, forEach:
Parsing Time: 386ms, 402ms

Edge , for, forEach:
Parsing Time: 3450ms, 3124ms
Memory: 548mb, 571mb

wafp.fbx (ASCII)

Chrome,for, forEach:
Parsing time: 1108ms, 1074ms
Memory: 92.5mb, 89mb

FireFox , for, forEach:
Parsing Time: 802ms, 663ms

Edge ,for, forEach:
Parsing Time: 6209ms, 5876ms
Memory: 687mb, 670mb

xsi_man_skinning.fbx (binary, uncompressed)

Chrome, for, forEach:
Parsing time: 1156ms, 1335ms
Memory: 64.5mb, 66mb

FireFox , for, forEach:
Parsing Time: 324mb, 280ms

Edge , for, forEach:
Parsing Time: 1417ms, 1367ms
Memory: 421mb, 241mb

Thoughts:

There seems to be very little difference between for and forEach, at least while using the latest browsers on desktop.

Deviation in results on FireFox and Chrome was very small (except for a couple of outliers, most of the runs I used to get the average were within 100ms of each other) - and it seems like using forEach is actually a little faster and more memory efficient. This is actually the same as what I found while making this PR, although I didn't want to make the claim without more rigorous testing! EDIT: not that I'm claiming that these tests are in any way rigorous 😆

Deviation in Edge was large (up a second difference between tuns on the ASCII file) so the results there are not so conclusive. Except that they show how slow Edge is compared to the other two.

[looeee]

Update: Brave Browser( Chromium based ) on Android

Did some testing on my old Moto G3 Android phone using the Brave browser which is based on Chromium v62.03.

Again, the results show very little difference. I would say these are a bit less accurate though as I kept getting "context lost" errors and deviation was up to a couple of seconds between runs.

wafp.bin.fbx (binary, compressed)

Brave, for, forEach:
Parsing time: 5234ms, 5536ms

wafp.fbx (ASCII)

Brave,for, forEach:
Parsing time: 13150ms, 13873ms

xsi_man_skinning.fbx (binary, uncompressed)

Brave, for, forEach:
Parsing time: 9599ms, 9251ms

[takahirox]

This result is very surprising and interesting to me...

I tried "Male Dynamic Pose.fbx" from mixamo on my Windows10 + Chrome, I realized the parsing time becomes 100ms faster (from 600ms to 500ms) by just changing few lines in genGeometry() of current FBXLoader.

Replacing

three.js/examples/js/loaders/FBXLoader.js

Lines 844 to 846 in b0e95eb

	for ( var polygonVertexIndex = 0; polygonVertexIndex < vertexIndices.length; polygonVertexIndex ++ ) {
	var vertexIndex = vertexIndices[ polygonVertexIndex ];

with

vertexIndices.forEach( function ( vertexIndex, polygonVertexIndex ) {

and replacing

three.js/examples/js/loaders/FBXLoader.js

Line 1114 in b0e95eb

}

with

} );

Seems like this part is the major (or almost all) performance gain from this PR.

If this result is true, we may should recommend to use forEach in many places for performance...
I'll take a look closer later...

[looeee]

This result is very surprising and interesting to me...

Yeah, so much that I don't totally trust the results 😆

I guess the takeaway here is that recent JIT compilers are basically black boxes and assuming things like "for loops are faster than forEach loops" is premature optimisation these days, even though it was once a sound assumption.

[mrdoob]

Alright! Lets give it a try! 😊

[mrdoob]

Thanks!

[looeee]

Sweet! If it ends up being an issue I'll change it back later 😊

[takahirox]

I tried further for vs forEach evaluation on my Windows10 + latest Chrome and let me share the result.

Even with for, performance improved by exporting the code in genGeometry() loop to function, like

function handleVertex( polygonVertexIndex, vertexIndex ) {

    ...

}

for ( var polygonVertexIndex = 0; polygonVertexIndex < vertexIndices.length; polygonVertexIndex ++ ) {

    handleVertex( polygonVertexIndex, vertexIndex );

}

In geoGeometry() case, the key thing seems like smaller scope rather than for or forEach. Smaller scope would generate more optimized code perhaps. (Yeah, smaller scope is one of the benefits from forEach
tho)

But I couldn't reproduce any cases where forEach is faster than for with other codes. I tried some benchmarks from very simple/small microbenchmark to benchmark as complex/large as genGeometry() but for is faster in all them. I'm very wondering what the actual factor in genGeometry() loop which makes forEach faster is...

So I think it's a bit too early to switch the default loop from for to forEach now. It'd better to keep current loop code style for now, we use for in general and use forEach (or other type loop) only when it's reasonable.

Let me know if you folks find the key factors which make forEach faster than for or any other cases where forEach is faster than for.

[WestLangley]

I tend to agree with @takahirox on this.

Regardless, we should have a well-defined policy. What is it?

[looeee]

Personally I'm in favour of forEach, except for in cases where performance is very important, since the code is more readable.

These test have at least shown that performance difference is likely to be just a few milliseconds on modern browsers, even with very large arrays (the wafp.fbx model I was testing has ~100,000 vertices).

[Mugen87]

I vote for ordinary for loops. Although forEach might be easier to read, we should generally tend to use language features that are focused on performance. From a pure technical point of view, simple for loops are much more efficient and produce less overhead.

So i clearly promote for as the default loop for three.js.

[Mugen87]

Let me know if you folks find the key factors which make forEach faster than for or any other cases where forEach is faster than for.

Indeed, that would be interesting to know.

[mrdoob]

Regardless, we should have a well-defined policy. What is it?

I vote for whatever is more performant. With a good balance on readability.

[WestLangley]

Regardless, we should have a well-defined policy. What is it?

I vote for whatever is more performant. With a good balance on readability.

That is a policy, but it is not well-defined. :-)

[looeee]

I've done a quick search through the codebase for forEach, map and reduce, excluding docs and included libs

forEach is used 111 times:
38 times in /test
71 times in/examples
1 time in /src (ShapeUtils)

map is used 53 times:
28 times in /test
25 times in /examples

reduce is used 3 time, in /examples

Also for good measure new Map is used 9 times, in the FBXLoader (it's also likely to be less supported and lower performance, so we should have a policy on this too).

My suggestion would be that we keep the strict policy for the /src folder, but relax it for other parts of the repo (so these methods can be used for files in the /test and /examples folder).

[mrdoob]

My suggestion would be that we keep the strict policy for the /src folder, but relax it for other parts of the repo (so these methods can be used for files in the /test and /examples folder).

Sounds good to me.

Also, right now is more important the awesome work you're doing on making the loader have excellent support. We can optimise things later.

[looeee]

We can optimise things later

Well, testing against the loader from r87, the wafp.bin.fbx file (compressed, binary) model I've been testing took 1158ms, now loads in 514ms in dev.

ASCII and uncompressed binary are about the same (maybe 100-200ms faster on larger models I'm testing), but since most people are using compressed binary FBX files, we can say that we have reduced loading times for FBX models by >50% in most cases.

...just in case you need to write some advertising blurb somewhere 😅

[mrdoob]

🎉🎉🎉