Performance improvements #2220

tonihele · 2024-03-13T15:13:58Z

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

There are few types of improvements:

Switching manual array copying to a system call (is this ok on Android??)
String concatenation / string format to use StringBuilder properly
Creating/copying collections switched to use bulk operation
Some early breaks to loops
Use Files to get Input/OutputStreams. At least on Windows this is proven to give faster stream implementation
Collection.toArray size thing (https://shipilev.net/blog/2016/arrays-wisdom-ancients/)

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

tonihele · 2024-03-13T15:16:03Z

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 · 2024-03-13T15:48:25Z

jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java

@@ -1398,7 +1398,7 @@ assert isValidNumber(fb) : "Invalid Matrix4f value " + uniform.getValue() + " fo
            case FloatArray:
                fb = uniform.getMultiData();
                assert isValidNumber(fb) : "Invalid float array value "
-                        + Arrays.asList((float[]) uniform.getValue()) + " for " + uniform.getBinding();
+                        + Collections.singletonList((float[]) uniform.getValue()) + " for " + uniform.getBinding();


While these two pieces of code are equivalent, I can't help but think this was not the intent of the original code. My reading is that this no different than just glomming the array directly into the string except that the cryptic float[].toString() is now wrapped in brackets.
Edit: meaning that both "before" and "after" the change are incorrect and the Arrays/Collections nonsense is unnecessary.

Arrays.toString used instead

pspeed42 · 2024-03-13T15:59:40Z

jme3-examples/src/main/java/jme3test/texture/TestSkyRotation.java

@@ -131,7 +131,7 @@ public void onAction(String name, boolean ongoing, float ignored) {
            System.out.print("rotate floor and sky leftward ...");
        } else if (name.equals("right")) {
            angle -= 0.1f; // radians
-            System.out.printf("rotate floor and sky spatials rightward ...");
+            System.out.print("rotate floor and sky spatials rightward ...");


While before/after are equivalent, I wonder if the original writer meant to have these all glom onto one line or if they left out the linefeed.

All system out prints in this class now have a linefeed

Linefeeds reverted

pspeed42 · 2024-03-13T16:01:44Z

jme3-networking/src/main/java/com/jme3/network/message/ChannelInfoMessage.java

@@ -69,6 +70,6 @@ public int[] getPorts() {

    @Override
    public String toString() {
-        return "ChannelInfoMessage[" + id + ", " + Arrays.asList(ports) + "]";
+        return "ChannelInfoMessage[" + id + ", " + Collections.singletonList(ports) + "]";


Another case where I think the authors original intent was to output the contents of the array and not just wrap the garbage string in brackets.
Edit: and looking where we are, I could be the original author and was just being dumb.

Arrays.toString used instead

pspeed42 · 2024-03-13T16:07:29Z

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 · 2024-03-13T16:17:29Z

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.

jme3-examples/src/main/java/jme3test/texture/TestSkyRotation.java

jme3-android/src/main/java/com/jme3/input/android/AndroidJoyInput.java

jme3-core/src/main/java/com/jme3/scene/shape/Surface.java

tonihele · 2024-05-14T08:16:02Z

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

stephengold · 2024-05-17T02:10:41Z

I haven't finished looking at all 107 files.

jme3-core/src/plugins/java/com/jme3/export/binary/BinaryExporter.java

stephengold

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.

stephengold · 2024-06-18T22:07:51Z

jme3-core/src/plugins/java/com/jme3/export/binary/BinaryImporter.java

'0 + offset' should be replaced by 'offset'.

stephengold · 2024-06-18T22:09:04Z

jme3-core/src/plugins/java/com/jme3/export/binary/BinaryOutputCapsule.java

@@ -744,8 +744,7 @@ else if (possibleMagic == DEFAULT_OBJECT)

        byte[] rVal = new byte[1 + size];
        rVal[0] = (byte) size;
-        for (int x = 1; x < rVal.length; x++)
-            rVal[x] = bytes[bytes.length - size - 1 + x];
+        System.arraycopy(bytes, bytes.length - size - 1 + 1, rVal, 1, rVal.length - 1);


Delete - 1 + 1 please.

stephengold · 2024-06-18T22:11:23Z

jme3-core/src/plugins/java/com/jme3/export/binary/ByteUtils.java

-                rVal[x] = bytes[x - (width - bytes.length)];
-            }
+            if (width - (width - bytes.length) >= 0)
+                System.arraycopy(bytes, width - bytes.length - (width - bytes.length), rVal, width - bytes.length, width - (width - bytes.length));


Please simplify width - bytes.length - (width - bytes.length) to 0 and width - (width - bytes.length) to bytes.length.

stephengold · 2024-06-18T22:18:05Z

jme3-core/src/test/java/com/jme3/shader/GLSLPreprocessorTest.java

        BufferedReader reader = new BufferedReader(new InputStreamReader(is));
        while (true) {
            String l = reader.readLine();
            if (l == null) break;
-            if (output != "") output += "\n";
-            output += l;
+            if (output.toString() != "") output.append("\n");


I think this code might be unreliable because it is using != to compare string.
It was unreliable before this change, for the same reason, but obfuscating it in the name of performance just makes it worse.
Either keep it unchanged, or if you want to improve it, replace output.toString() != "" with output.length() > 0.

Or if we are in the modern Java, then we can use isEmpty(). Can't remember when it came...

As of Java 22, there's no StringBuilder.isEmpty().
But String.isEmpty() dates back to Java 1.6, so that would work.
I just assume it would be more efficient to test the length property of the StringBuilder than to convert it to a String and then test its length property, regardless of whether you use String.isEmpty() or String.length.

I think you are right about that.

stephengold · 2024-06-18T22:22:24Z

jme3-terrain/src/main/java/com/jme3/terrain/heightmap/FluidSimHeightMap.java

-            for (int x = 0; x < size; x++) {
-                heightData[x + y * size] = tempBuffer[curBuf][x + y * size];
-            }
+            System.arraycopy(tempBuffer[curBuf], 0 + y * size, heightData, 0 + y * size, size);


Please delete the 0 + from 2 expressions.

tonihele · 2024-06-19T05:44:39Z

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 · 2024-06-19T06:08:49Z

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 · 2024-06-19T07:12:19Z

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 · 2024-06-19T16:54:48Z

@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.

tonihele added 2 commits March 13, 2024 16:56

Performance improvements (static code analysis)

6d1ecc3

Remove test files

db866c2

Revert optimization

f381097

pspeed42 reviewed Mar 13, 2024

View reviewed changes

tonihele added 3 commits March 17, 2024 14:30

Print to own lines

45c4996

Print the contents of the array

730d66e

Print the contents of the array

87a5baa

tonihele requested a review from pspeed42 March 17, 2024 12:34

Fix linefeed

47a9e95

stephengold reviewed Mar 17, 2024

View reviewed changes

jme3-examples/src/main/java/jme3test/texture/TestSkyRotation.java Outdated Show resolved Hide resolved

No linefeed

b498f3f

tonihele requested a review from stephengold March 17, 2024 18:37

stephengold reviewed Mar 17, 2024

View reviewed changes

jme3-android/src/main/java/com/jme3/input/android/AndroidJoyInput.java Show resolved Hide resolved

stephengold reviewed Mar 17, 2024

View reviewed changes

jme3-core/src/main/java/com/jme3/scene/shape/Surface.java Outdated Show resolved Hide resolved

No wildcard imports

f4206ee

tonihele requested a review from stephengold May 14, 2024 08:15

stephengold reviewed Jun 1, 2024

View reviewed changes

jme3-core/src/plugins/java/com/jme3/export/binary/BinaryExporter.java Outdated Show resolved Hide resolved

Simplify equations

e6c5018

tonihele requested a review from stephengold June 15, 2024 09:00

stephengold requested changes Jun 18, 2024

View reviewed changes

tonihele added 7 commits September 22, 2024 13:31

Simplify the arraycopy parameters

c3e8791

Compare size rather than empty string

2e05800

Try catch resources and use Files to instantiate output/input streams

aae2469

Simplify read string

616e0c7

Revert

cd3ae35

Revert nbytes

f749352

Try catch resources and use Files to instantiate output/input streams

6e13257

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements #2220

Performance improvements #2220

tonihele commented Mar 13, 2024 •

edited

Loading

tonihele commented Mar 13, 2024

pspeed42 Mar 13, 2024 •

edited

Loading

tonihele Mar 17, 2024

pspeed42 Mar 13, 2024

tonihele Mar 17, 2024

tonihele Mar 17, 2024

pspeed42 Mar 13, 2024 •

edited

Loading

tonihele Mar 17, 2024

pspeed42 commented Mar 13, 2024

tonihele commented Mar 13, 2024

tonihele commented May 14, 2024

stephengold commented May 17, 2024

stephengold left a comment

stephengold Jun 18, 2024

stephengold Jun 18, 2024

stephengold Jun 18, 2024

stephengold Jun 18, 2024

tonihele Jun 19, 2024

stephengold Jun 19, 2024

tonihele Jun 19, 2024

stephengold Jun 18, 2024

tonihele commented Jun 19, 2024

stephengold commented Jun 19, 2024

tonihele commented Jun 19, 2024 •

edited

Loading

stephengold commented Jun 19, 2024 •

edited

Loading

Performance improvements #2220

Are you sure you want to change the base?

Performance improvements #2220

Conversation

tonihele commented Mar 13, 2024 • edited Loading

tonihele commented Mar 13, 2024

pspeed42 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pspeed42 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pspeed42 commented Mar 13, 2024

tonihele commented Mar 13, 2024

tonihele commented May 14, 2024

stephengold commented May 17, 2024

stephengold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonihele commented Jun 19, 2024

stephengold commented Jun 19, 2024

tonihele commented Jun 19, 2024 • edited Loading

stephengold commented Jun 19, 2024 • edited Loading

tonihele commented Mar 13, 2024 •

edited

Loading

pspeed42 Mar 13, 2024 •

edited

Loading

pspeed42 Mar 13, 2024 •

edited

Loading

tonihele commented Jun 19, 2024 •

edited

Loading

stephengold commented Jun 19, 2024 •

edited

Loading