javagl
commented
1 year ago I may have to take a closer look at the exact changes. From quickly skimming over the changes, it seems to be related to two things that one could call "high-level implementation decisions":
1:
When dealing with such "bulk" data, it can often be useful to have two flavors of functions. One that allocates a new buffer, and one that writes the result into a given buffer. For example, as in
FloatBuffer vertices = something.getVertices();
and
FloatBuffer vertices = allocate();
something.getVertices(vertices);An aside: It often makes sense to combine this into a "hybrid" function, like
// Fills the target and returns it
// When the target is null, then a new buffer is created, filled, and returned
FloatBuffer getVertices(FloatBuffer target) {
FloatBuffer result = target;
if (result == null) result = allocate();
...
return result;
}but this isn't the pattern that is used here.
You changed the function
public static void getVertices(ReadableObj obj, FloatBuffer target) { ... }
// to
public static void getVertices(ReadableObj obj, float[] target) { ... }First of all, that's a breaking API change and I won't accept this in this exact form.
But of course it would be easy to work around that: There could be the original getVertices function, and - following the naming pattern that is already used in general - there could be another one like getVerticesArray(... float[] target).
The change is not only breaking, but also reduces the versatility. With the FloatBuffer one, you could have
FloatBuffer verticesTwice = allocate(12 * 2);
ObjData.getVertices(obj, verticesTwice);
ObjData.getVertices(obj, verticesTwice);e.g. for filling the buffer with vertices from multiple OBJs. In order to "emulate" this possibility, you'd have to change the method to
public static void getVerticesArray(
ReadableObj obj,
float[] target,
int offset) // Add an 'offset' parameter, saying where to start writing into the arraySo the FloatBuffer one is more flexible in this regard. And this flexibility leads to the other design option:
2:
When dealing with "bulk data of primitive values", then there usually are the options of using a primitive array like float[], or a buffer like FloatBuffer. And the aforementioned flexibility is related to the question (that I always try to think about, not only in this particular case):
When there are two design options A and B, which one can emulate the other?
And as a rule of thumb:
- When A can emulate B: Use A
- When B can emulate A: Use B
- When both can emulate each other: Find other criteria for the decision (and that may be 'performance' - more on that below)
- Neither can emulate the other: These are not two 'options', but entirely different things ...
The point is (and that you may have noticed in the methods of the ObjData class that you modified):
A: When you have a method that fills a FloatBuffer target, then it's trivial to fill a float[] array with that:
float[] array = new ...;
fill(FloatBuffer.wrap(array));B: When you have a method that fills a float[] target array, then ... it can be a little bit quirky and inconvenient when you want to fill a FloatBuffer...
FloatBuffer buffer = alloc();
float array[] temp = new ...;
fill(temp, 0);
buffer.put(temp);(allocating the data twice, just to shove it from the array into the buffer)
An aside: For buffers, there's the additional design dimension of whether you want to heap-allocated buffers or 'direct' buffers. For anything that is related to OpenGL/native libraries, you usually want 'direct' buffers...
A detail:
The float[] getValues(); method in the FloatTuple looks innocent. After all, "it's just a getter"....
But ... it makes the state mutable. You could call
tuple.getValues()[1] = Float.NaN;
Until now, the FloatTuple is immutable.
Beyond that: I know that it's very unlikely that there will ever be a different implementation of the FloatTuple interface than the DefaultFloatTuple. But imagine there was an implementation that did not store the values in a float[] array. If there was an implementation that was based on a FloatBuffer (or if I wanted to change the DefaultFloatTuple to store the values in a FloatBuffer for some reason), then this method would bite back, painfully:
@Override
public float[] getValues() {
float[] result = new float[3]; // Potentially called millions of times...
result[0] = data.get(0);
result[1] = data.get(1);
result[2] = data.get(2);
return values;
}This would probably eat up many performance gains that one might achieve elsewhere...
This leads to the main point:
The main claim of this PR is to be a "performance optimization" and make things "2x as faster" (sic).
I doubt that :)
Performance tests are difficult. Always. And they are particularly difficult on a Virtual Machine. And they are even more difficult on a Virtual Machine that has a Just-In-Time-Compiler.
But as a very quick test (to be taken with a grain of salt for the above reasons), I created this:
import java.io.FileInputStream;
import java.nio.FloatBuffer;
import java.nio.IntBuffer;
import de.javagl.obj.Obj;
import de.javagl.obj.ObjData;
import de.javagl.obj.ObjReader;
public class ObjPerformanceTestPR30
{
public static void main(String[] args) throws Exception
{
String s = "example.obj";
Obj obj = ObjReader.read(new FileInputStream(s));
double sum = 0;
long before = System.nanoTime();
for (int i=0; i<1000; i++)
{
IntBuffer indices = ObjData.getFaceVertexIndices(obj);
FloatBuffer vertices = ObjData.getVertices(obj);
sum += indices.get(0);
sum += indices.get(1);
sum += vertices.get(0);
sum += vertices.get(1);
}
long after = System.nanoTime();
System.out.println("Took "+(after-before)/1e6+" ms with sum "+sum);
}
}I ran this with some arbitrary ~10MB OBJ file that I had lying around here.
With master, the output for me was
Took 2271.6398 ms with sum -7199.090003967285
With this branch, the output for me was
Took 2494.5546 ms with sum -7199.090003967285
Of course, the results on another VM (e.g. on Android) may be completely different. But still, I'd like to see the claim that the new approach is "noticably faster" (on most or some VMs) to be backed by examples...
PhpXp
It's faster to create an array with data and then bulk
put()it to a buffer. For loops were also optimized. Code was tested on an Android phone and after the optimisations, buffers are created about 2x faster.Public function signatures were changed, hopefully this isn't a big problem. For example in
getFaceVertexIndices(), you shouldn't be adding one value at a time to the buffer anyway, that's slow. Note that the code isn't final, Javadoc still has to be updated.