mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.69k stars 35.3k forks source link

Performance regress in r53? #2638

Closed alteredq closed 11 years ago

alteredq commented 11 years ago

If I remember well, these examples used to be solid 60 fps:

http://mrdoob.github.com/three.js/examples/webgl_geometry_hierarchy.html http://mrdoob.github.com/three.js/examples/webgl_geometry_hierarchy2.html http://mrdoob.github.com/three.js/examples/webgl_interactive_cubes.html http://mrdoob.github.com/three.js/examples/webgl_interactive_draggablecubes.html

With the current lib these can now be much slower, some of them with some viewing angles down to ~20 fps.

Other performance stress tests seem to be more or less unchanged, which would suggest this performance regress could be related to some change in scene graph processing or math (as opposed to rendering).

All these slowed down examples do either a lot of computations on scene graph or use Projector / Ray a lot.

Also lensflares example is much slower, but that one was always kinda hit or miss due to randomness with CPU <=> GPU synchronization so it's probably unrelated.

Edit: upon some poking with profiler this seem to be more tricky and not related to scene graph or math.

Edit2: ok, I missed the obvious connection - all these examples use cubes. There is something wrong with CubeGeometry, replacing Suzanne in performance test with cube drops performance 10x.

Yup, it's CubeGeometry, using SphereGeometry instead makes it fast.

alteredq commented 11 years ago

Bah, I think I found the culprit - thanks to marking each cube face with a different material index all cubes are broken down into 6 individual sides, thus increasing the GL API call load 6x.

We will need to find some other way how to mark cube sides (which is useful and needed, I already miss px/nx/py/ny/pz/nz gang, these troublesome material indices are just poor substitute for this lost functionality).

mrdoob commented 11 years ago

So WebGLRenderer is splitting the geometry even if they're not using MeshFaceMaterial?

alteredq commented 11 years ago

Yes, that's how it always was. It's material indices which indicate mesh splitting.

Before, CubeGeometry was having all material indices undefined unless material array was passed in, so independently of turning faces on/off it was always just a single geometry.

Edit: I checked and it should be possible to change it so that splitting happens just when there is MeshFaceMaterial on the first render. Should we?

mrdoob commented 11 years ago

Edit: I checked and it should be possible to change it so that splitting happens just when there is MeshFaceMaterial on the first render. Should we?

I think so, yes...

alteredq commented 11 years ago

Ok, changed, we are fast again ;)

This should probably go also to master branch.

mrdoob commented 11 years ago

Yep. I'll do some git hackery again :)

mrdoob commented 11 years ago

Btw, I've been doing some git undos and tricks. So eventually you'll need to do force pull to properly sync. By now I'll cherry pick your commits.

mrdoob commented 11 years ago

Oh, ugh. There is quite a mess now... We'll see what I can do.

alteredq commented 11 years ago

Uff, I meant just this single commit, attributes ones are bit too scary for master.

You could just ignore my other commits and copy paste changes from this commit, these are quite well localized.

mrdoob commented 11 years ago

Yeah, but because you put the builds in the same commit I can't cherry pick.

mrdoob commented 11 years ago

Do you mind branching out master and doing a commit to it with only the code?

alteredq commented 11 years ago

Ok, I'll try. Hehe, I didn't update my master in over a year ...

alteredq commented 11 years ago

Ok, committed in master-facesplit-fix branch.

mrdoob commented 11 years ago

Ok. Merged into master. dev is now also synced with master which makes your attribute related commits unsyncable... Maybe you can do a fresh branch of dev and push those commits?

alteredq commented 11 years ago

I can try just to merge my dev with your dev ...

mrdoob commented 11 years ago

Nope. I've done some trickery. I've undo'ed some commits... :P

alteredq commented 11 years ago

Which ones? I just did the merge and it seems to went quite smoothly.

mrdoob commented 11 years ago

There are 3 "r54dev" commits in your repo, only 1 of them exist in my repo. But it's ok. The designer in me is being too picky ;)

alteredq commented 11 years ago

These are so good commits I need many of them ;)

mrdoob commented 11 years ago

Haha!

tarelli commented 11 years ago

All my geometries are CylinderGeometry so maybe there's a problem with those too? See #2712

alteredq commented 11 years ago

@tarelli Could you try with r54dev? In theory this should have been already fixed.

tarelli commented 11 years ago

No luck with r54dev, 3fps. Added a screenshot here https://www.dropbox.com/sh/0azw2owz95ogtur/aMxD82iReA?lst

alteredq commented 11 years ago

Hmmm, could you post a live link somewhere? Seems like we missed some use case in refactoring of MeshFaceMaterial.

tarelli commented 11 years ago

I'm working to deploy the dev version somewhere, will let you know as soon as I have a link, in meanwhile it might be useful for you to look at the sources: https://github.com/NeuroML-WorkInProgress/org.neuroml.visualiser/blob/master/WebContent/js/OW.js have a look at OW.getCylinder and OW.getThreeObjectFromJSONEntity which is pretty much where I build the scene. Also if you see anything stupid that I'm doing please let me know ;)

alteredq commented 11 years ago

What pops for me:

var c = new THREE.CylinderGeometry(radiusTop, radiusBottom, cylHeight, 6, false);

Looks like mismatch in parameters (false in place of heightSegments)?

THREE.CylinderGeometry = function ( radiusTop, radiusBottom, height, radiusSegments, heightSegments, openEnded )
tarelli commented 11 years ago

Good catch, I changed that, still no luck though.

alteredq commented 11 years ago

If it's too much trouble to put it online, here is what you could try:

With such huge performance drop there should be something obvious popping up. That's for example how I caught CubeGeometry being broken into pieces, there were zillions of drawcalls where there should have been few.

tarelli commented 11 years ago

I added 4 files with slow and fast inspection. Unfortunately I don't know how to dump the entire trace, it doesn't let me copy and paste, hope in those two fragments you can see something. Drawing seems to happen more often in R53. https://www.dropbox.com/sh/0azw2owz95ogtur/aMxD82iReA?lst

tarelli commented 11 years ago

Also there seems to be 79K triangles drawn by the last call in the slow version compared to 4K in the fast version, that seems wrong?

alteredq commented 11 years ago

Also there seems to be 79K triangles drawn by the last call in the slow version compared to 4K in the fast version, that seems wrong?

That's indeed very suspicious, though it could be just accidental, for example if you managed to take snapshots with different order of rendering so big object got rendered earlier or later.

But a number of GL API calls seems to be similar so it looks like it could be indeed about having many more triangles. Or also it could be having more GL state switches.

One thing you can try is to check internal three.js stats (either dumping this somewhere from your code, or just type it into Chrome console if you have renderer object accessible globally):

renderer.info

https://github.com/mrdoob/three.js/blob/master/src/renderers/WebGLRenderer.js#L77

tarelli commented 11 years ago

Added two more files, stats are identical for the two versions, see: https://www.dropbox.com/sh/0azw2owz95ogtur/aMxD82iReA?lst

alteredq commented 11 years ago

Hmmm, if number of drawcalls and number of triangles are the same, then it looks like it could be GL state changes. This can be tricky.

We did change recently sorting to be stable for when objects are in the same position. If your application before accidentally depended on some particular order of objects this could have changed.

One thing you can try is turning off sorting in both slow and fast versions and see if there is any change:

renderer.sortObjects = false;
tarelli commented 11 years ago

I tried that, same performance unfortunately. On a side note I'm glad you did some work on that since I had some occasional flickering with meshes occupying the same position :)

alteredq commented 11 years ago

Just to be sure - could you try setting all objects to exactly the same material?

Also anything suspicious in Chrome profiler?

tarelli commented 11 years ago

Same problem using the same material for everything. Added a screenshot to the folder with profiling info: https://www.dropbox.com/sh/0azw2owz95ogtur/aMxD82iReA?lst

alteredq commented 11 years ago

Interesting, could you make profiler show percents?

This doesn't look like rendering issue, seems like massive amounts of time are spent on scene graph computations.

Do you create enormous amounts of objects (and they may be invisible because not that many show in renderer.info)?

tarelli commented 11 years ago

Added a file with percentages. In that example I create about 10,000 cylynder geometries. I then merge them and I endup with about 300 meshes.

alteredq commented 11 years ago

Well, it looks like somehow you end up with 10,000 meshes. At least that would explain such performance profile.

Or possibly even more than 10,000. Our performance stress test example handles 5,000 meshes without much trouble. Or maybe your objects are in some sort of complicated hierarchy. But definitely try to inspect your scenegraph. Dump scene into console and check what's inside.

tarelli commented 11 years ago

Added profiling of R51, have a look, it's pretty interesting!

alteredq commented 11 years ago

Yes, that's how it should look like. Somehow somewhere with a new version there must be a lot of junk added to the scene graph.

Are you 100% sure you didn't do any other changes when switching lib versions? Please double-check both your app specific code and also migration guide:

https://github.com/mrdoob/three.js/wiki/Migration

tarelli commented 11 years ago

Added R51R53Scene comparison, same number of objects/children. I also sampled the same mesh in both and they have same number of faces, vertices.

tarelli commented 11 years ago

I am looking at the same version, I have one html which points to R53 and one to R51, everything else is the same. The only thing that I'm using that changed from Migration doc is scene traversing, but it's a function not used unless I do something specific which I'm not doing (and I changed it to be object.travers just in case and still it doesn't make a difference as expected).

alteredq commented 11 years ago

Hmmm, can you drill down deeper? Maybe some of these children have their own children?

You can check from console:

THREE.Object3DIdCount
THREE.Object3DLibrary

Also try to comment out anything with Ray.

alteredq commented 11 years ago

Yes, traversing is suspicious, that's how you could generate tons of stuff accidentally.

tarelli commented 11 years ago

R51:

THREE.Object3DLibrary undefined THREE.Object3DIdCount undefined THREE.Object3DCount 15008

R53: THREE.Object3DIdCount 15008 THREE.Object3DLibrary Array[15008]

alteredq commented 11 years ago

Well, these 15K objects is what could explain poor performance. The question is why in r51 this doesn't matter.

Try checking this in console:

scene.__objects
tarelli commented 11 years ago

In both:

OW.scene.__objects Array[304]

tarelli commented 11 years ago

Another measurement. Heap memory usage seems to be twice as much in R53, 700MB vs 300MB in R51

tarelli commented 11 years ago

Could it be related to #2633?