search

jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.74k stars 1.12k forks source link

Performance improvements #2220

Open tonihele opened 3 months ago

tonihele commented 3 months ago

Performance improvements. From static code analysis. All Java 8 compatible (is this the official code level?).

There are few types of improvements:

This should be all pretty safe stuff. I did not benchmark this separately at all. These are based on static code analysis.

tonihele commented 3 months ago

Note that if merged, use SQUASH and merge. I accidentally added some test files in the process. The Squashing will rewrite this so that that oopsie never happened... right? Anyway, nothing dirty, Jaime heads in various formats

pspeed42 commented 3 months ago

re: your description when it says "Arrays.asList size thing" I think you mean "Collection.toArray() size thing"

Curious: what static code analysis was used?

tonihele commented 3 months ago

Curious: what static code analysis was used?

I used IntelliJ's.

re: your description when it says "Arrays.asList size thing" I think you mean "Collection.toArray() size thing"

Correct. Fixed to description.

tonihele commented 1 month ago

Is this now ok to merge? I believe I did all the requested changes?

stephengold commented 1 month ago

I haven't finished looking at all 107 files.

tonihele commented 2 weeks ago

I didn't get very far in my review before losing enthusiasm. Hope my review is helpful. Many issues I found involve System.arrayCopy(). You may want to self-review those changes.

Thanks, I'll go through them. These are all changes by automation.

stephengold commented 2 weeks ago

I didn't get very far in my review before losing enthusiasm. Hope my review is helpful. Many issues I found involve System.arrayCopy(). You may want to self-review those changes.

Thanks, I'll go through them. These are all changes by automation.

Submitting a huge PR that's 100% auto generated and then relying on human volunteers review it seems abusive. And perhaps detrimental to the project as a whole.

tonihele commented 2 weeks ago

I didn't get very far in my review before losing enthusiasm. Hope my review is helpful. Many issues I found involve System.arrayCopy(). You may want to self-review those changes.

Thanks, I'll go through them. These are all changes by automation.

Submitting a huge PR that's 100% auto generated and then relying on human volunteers review it seems abusive. And perhaps detrimental to the project as a whole.

I'm terribly sorry. I did of course go through it myself prior submitting it. And my motivation is to help the project as a whole. And I feel that I am one of the abused human volunteers that you referenced, having been here for a long time too. And to be perfectly clear, I don't feel abused by this PR, any comment or anything, just including myself to this sorry lot :D

My point perhaps was the scope of this PR. I'm more than happy to discuss and make changes you guys request, no problem there. Just kinda a defensive point that the focus point was only on the performance of these scattered lines. Not to make existing problems go away or make actual changes in behavior. Of course there is no intention of making the code less readable either.

And thank you for your review!

stephengold commented 2 weeks ago

@tonihele, I realize of course that you're also a volunteer, and I greatly value and respect you and your past contributions to the project. If this PR had come from someone I didn't know and respect, I would have totally ignored it or closed it as "won't fix." As it is, I've neglected it. I realize now I haven't been forthcoming about my reasons for that neglect, and I apologize for my silence. You deserve better communication, so I'll try to explain myself.

Engine performance is important, but micro improvement efforts should focus on the inner loops of practical workloads. Furthermore, I doubt optimizations auto-generated by static analysis will measurably improve the performance of JME. I tried to explain this to xtonik last year (https://hub.jmonkeyengine.org/t/newcomer/46680) and didn't do a great job.

A lot of Engine code is semi-obsolete, rarely executed, and/or executed only during testing or initialization or error conditions. Automated static analysis doesn't take such factors into account.

Furthermore, many micro-optimizations reduce readability. For example, I'd rather read a for loop than an invocation of System.arrayCopy(). I read and write for loops daily and find them very readable. OTOH I find arrayCopy() very opaque. It has 5 positional arguments (3 of which are integers) that are easily confused. I use it so rarely that I forget its precise semantics: what happens if an argument is negative, what happens if the arrays are too short, and what happens if src and dest are the same array or are of different types.

IMO, the time and energy of the core developers is better spent on bugs that visibly affect apps. If you enjoy spending your time on micro-optimizations, that's what you should do. But repeatedly, publicly, and specifically requesting my review drags me into work I don't enjoy and have little talent for, all for benefits I don't believe in.

Again, I'm sorry it's taken me so long to articulate my underlying concerns about this PR, and I apologize for that.