There are many places I’ve found in jME3 to improve performance, both in memory usage & CPU usage. I’ve tested changes through the profiler to see significant improvements. The first I’d like to share requires changes to the Material, Technique & DefineList classes. Basically, temporary iterators are being created that is taking up a bunch of CPU & memory in the DefineList’s “equalParams” function. We don’t need to make these iterators – we can access the ListMap’s values directly since there is a backing array. The start of the function can be changed to this:
[java] public boolean equalsParams(ListMap params, TechniqueDef def) {
int size = 0;
for(int i = 0; i < params.size() ; i++ ) {
MatParam param = (MatParam)params.getValue(i);[/java]
… it requires passing the ListMap to this function, instead of Collection<MatParam>. To make that change, Material needs to return its ListMap and Technique needs to pass this variable along. Anyway, I’m benefiting from this change (and more) in my local repository & I’d like to push at least this one to the SVN.
In patch 2 there are some optimalizations which reduce amount of garbage generated around these areas. I’m not sure if it will still apply cleanly, but you can take a look there.
Not sure if patch1 was applied - it was already long time ago.
Good find. These solutions need to be worked into the engine quicker, so people like me don’t need to solve the same problems again. I’ll push my changes to the SVN, which are clean. Before anyone says “garbage collection is fast & this isn’t a concern”: it might not be for you, but apparently it is for many people:
I also have a solution for shadows, but it isn’t as clean & I’d like other input. Basically, I create a pool of BoundingVolumes to use, instead of always creating new ones:
[java] // re-use BoundingVolumes so we don’t keep piling on the memory usage
private static BoundingVolume[] tempBVs;
private static int BVindex;
… which fixes garbage collection problems and is very fast. However, I’m wondering if the fixed 2048 number of BoundingVolumes works universally… works for me.
On the other hand, people are mostly hit by long-lived garbage, not by immediate garbage - and at least my patches were reducing short-lived garbage. I have done them mostly to be able to profile my own code - without them, ‘garbage garbage’ from engine was dominating ‘real garbage’ from user code. Plus, I had trial version of profiler, so I had to use it
You can add the NetBeans profiler as a plugin to the jME3 SDK, which is free to use. It has been very helpful, and I’ve also noticed the garbage from the engine greatly overwhelms any garbage in my code. I’ve also used the CPU profiler to clean up other spots in the engine – for example, using FastMath.abs(x) is slower than just doing “(x <= 0.0f) ? 0.0f - x : x;”, but you see FastMath.abs(x) used everywhere. I’d love to start submitting those changes too in a few key places.
I really don’t understand why FastMath.abs would be slower. Be sure that you are doing proper measurements - this kind of code was inlined by Hotspot already 10 years ago. Are you using server jvm and allowing it to warm up? In any case, I really doubt that any difference here would be visible in real game profile.
There is a difference between cutting 80% of garbage by changing few lines versus optimizing 0.001% of speed by making 100 lines unreadable. We need to balance these things.
Basically, skipping FastMath.abs() inlines the functionality – no function calls needed. It probably isn’t that noticeable for most projects, but in tight loops it adds up. I noticed it in shadow processing, if I remember correctly. It is a bit harder to read, but I modify each line like this:
Anyway, this is relatively minor to other improvements. For example, Quaternion was taking up a significant amount of memory in toRotationMatrix. I made a non-temporary Vector3f to store results, so one doesn’t need to be created every call:
Interesting. I do remember using the profiler & seeing a modest CPU reduction when not using FastMath.abs(), so I stuck without it. Anyway, not big enough to push changes to the main SVN, but people looking to squeeze a few more FPS might find some benefit there.
I’m considering pushing the toRotationMatrix one, though. It does add a null check to the function, but that is to prevent a new Vector3f being created with every Quaternion unnecessarily – it instead gets created when needed only once for all calls to toRotationMatrix(…)
My bad about the patches Abies posted, I should have merged them.
@phr00t what @abies says about warming up, micro benching is trickier than one may think, and have a good design for the test is crucial to have meaningful results.
I can be wrong , but I seriously doubt java7 is not inlining this kind of things through hot spot. Byte code won’t give you what really happens in the long run.
About the shadows patch is that in ShadowUtil? If yes 2048 seems a bit excessive, usually one would use some kind of growing pool instead of static instances.
Also your pool system doesn’t guarantee to have a “free” bounding volume as it cycles through the array to avoid freeing instance (i suppose). I agree it may not occur in the general case since you have 2048 instances…but what if you need more boundingBox than 2048…you will just get the first one and have some silent failure… honestly that’s not the way to do it.
From what I gathered there are 3 instanciations of boundingBox in ShadowUtil.updateShadowCam (that’s 3 per frame), but I’d really like to see the numbers about the garbage it generates in the end, and how it’s managed by java.
@phr00t said:
I'm considering pushing the toRotationMatrix one, though. It does add a null check to the function, but that is to prevent a new Vector3f being created with every Quaternion unnecessarily -- it instead gets created when needed only once for all calls to toRotationMatrix(...)
Please, don't. We have tempVars for this, so please use them.
@nehon said:
Please, don't. We have tempVars for this, so please use them.
Then jMonkeyEngine should follow your own advice & toRotationMatrix should use tempVars to get the same benefit as in my code.
@nehon said:
My bad about the patches Abies posted, I should have merged them.
@phr00t what @abies says about warming up, micro benching is trickier than one may think, and have a good design for the test is crucial to have meaningful results.
I can be wrong , but I seriously doubt java7 is not inlining this kind of things through hot spot. Byte code won’t give you what really happens in the long run.
About the shadows patch is that in ShadowUtil? If yes 2048 seems a bit excessive, usually one would use some kind of growing pool instead of static instances.
Also your pool system doesn’t guarantee to have a “free” bounding volume as it cycles through the array to avoid freeing instance (i suppose). I agree it may not occur in the general case since you have 2048 instances…but what if you need more boundingBox than 2048…you will just get the first one and have some silent failure… honestly that’s not the way to do it.
From what I gathered there are 3 instanciations of boundingBox in ShadowUtil.updateShadowCam (that’s 3 per frame), but I’d really like to see the numbers about the garbage it generates in the end, and how it’s managed by java.
Yes, I believed there was a better, more universal way of handling the ShadowUtil problem & you are right. My system has been working for my situation, but a very similar system with your concerns accounted for should be put in place as soon as possible.
@phr00t said:
Then jMonkeyEngine should follow your own advice & toRotationMatrix should use tempVars to get the same benefit as in my code.
Yes, I believed there was a better, more universal way of handling the ShadowUtil problem & you are right. My system has been working for my situation, but a very similar system with your concerns accounted for should be put in place as soon as possible.
Dude you make patches…make them right.
I couldn’t care less that it works for 3089 or whatever you’re working on. We didn’t gave you commit access to push your own changes in the engine to your convenience, I’d like you to at least wait for us to discuss your changes before committing them.
You have valid points here, but the way you solve them and force them into the engine is not right.
@nehon said:
Dude you make patches...make them right.
I couldn't care less that it works for 3089 or whatever you're working on. We didn't gave you commit access to push your own changes in the engine to your convenience, I'd like you to at least wait for us to discuss your changes before committing them.
You have valid points here, but the way you solve them and force them into the engine is not right.
I haven’t pushed any of FastMath.abs, ShadowUtil updates or toRotationMatrix ones because I’m doing just what you suggest: discussing the changes.
I know you don’t care if it works for 3089 or not – that is why I don’t push those updates, but I take time to discuss them here.
I’m not forcing anything on the engine, but I am trying to be efficient and avoid improvements not making them into the engine at all due to bureaucracy. For example, the DefineList change was very small, but saw huge improvements & fixes problems existing for months.
@phr00t said:
I haven't pushed any of FastMath.abs, ShadowUtil updates or toRotationMatrix ones because I'm doing just what you suggest: discussing the changes.
I know you don’t care if it works for 3089 or not – that is why I don’t push those updates, but I take time to discuss them here.
I’m not forcing anything on the engine, but I am trying to be efficient and avoid improvements not making them into the engine at all due to bureaucracy. For example, the DefineList change was very small, but saw huge improvements & fixes problems existing for months.
FYI: It wasn’t bureaucracy in this case. It just needed a bump. Since the patches were posted as a download link and Nehon had already looked at them, I think the rest of us just moved on.
Point here: while it’s good to post download links to avoid patch formatting issues. It’s also good to post the diffs right to the thread so that ‘we of little time’ can read them. So, thanks for doing that Phr00t. These changes/discussion have been much easier to follow because of that.
I looked over the commits so far and they are good. We will have to move them over to the new build for them to really take effect but they look fine to me.
@pspeed said:
FYI: It wasn't bureaucracy in this case. It just needed a bump. Since the patches were posted as a download link and Nehon had already looked at them, I think the rest of us just moved on.
Point here: while it’s good to post download links to avoid patch formatting issues. It’s also good to post the diffs right to the thread so that ‘we of little time’ can read them. So, thanks for doing that Phr00t. These changes/discussion have been much easier to follow because of that.
I looked over the commits so far and they are good. We will have to move them over to the new build for them to really take effect but they look fine to me.
The toRotationMatrix change look good? I’d like to submit that.
I’m also curious about these new builds and the new fangled “JOGL” stuff… wonder if I should be preparing for it… maybe for the next game? Another thread, perhaps…
Your 3 changes looks fine (already committed), but they touches classes that are very central to the engine, so it raised the red flag when I saw them. As a rule of thumb you should post this kind of changes before committing them.
The toRotationMatrix change looks ok, go ahead.
I know I’m not as active as before but life’s a bitch. I’ll eventually commit those patches (abies ones), one main reason is that you guys won’t stop to remind me to do it. But one point of the core team is to centralize contributions and we don’t want to discover issues months later due to commits we weren’t aware of.
Looks like the only potentially useful stuff remaining in abies’s patches are string caching stuff, since the other things are now fixed. Looks like we will still need a solution for the BoundingVolumes in ShadowUtil, though. Not sure when I will get to a better solution, since I’ve hacked one that works for me, but I do like contributing to jME3 when I can.
