static vector operations and ReadableVector3f

[Jens v.P.]

Hi,

since I'm using the immutable/mutable paradigm for avoiding unnecessary copy operations (and unexpected modifications), I'm using ReadableVector3f a lot. Now I've found that Vector3f's static operations, e.g. add(..), are expecting Vector3f parameters instead of ReadableVector3f, even if the parameter is only read.

I assume that these operations are using Vector3f for performance reasons. Or is this simply a bug? Is it possible to change the method signatures to add(ReadableVector3f, ReadableVector3f, Vector3f) (and the other methods respectively)? Or add new operations with these signatures?

Jens

PS: It's really a pity that Java doesn't support "const" as it is defined in C++ ...

PPS: Should Sourceforge's feature tracker be used instead of this forum? The trackers seem so empty...

[princec]

It's a design bug.

Cas

[Jens v.P.]

OK, so maybe I should submit a bug to LWJGL's bug tracker?

Talking about the vector classes, the following methods could be moved to interface ReadableVector3f, since they do not change the state of a Vector3f:
- normalise(Vector3f)
- negate(Vector3f)

Jens

[Matzon]

The best way to get this fixed, is to submit a patch
Using the forum is preferred.

[Jens v.P.]

OK, here is the patch, based on version 2909.

I replaced Vector?f with ReadableVector?f in all static Vector?f methods where possible. I did not pulled up normalise(Vector?f) and negate(Vector?f) since the hierarchy of the ReadableInterfaces does not correspond with the hierarchy of the implementation. That is, ReadableVector3f extends ReadableVector2f, but Vector3f does not extend Vector2f.

Since I used a tool (Plug-In Poor Man's Quick Fix) for changing the access method within these functions (from foo.x to foo.getX()), I think everything should be ok.

[Jens v.P.]

OK, even with a tool a fool is still a fool, so I added some test classes ensuring I didn't mixed something up. Do you have a test system with automatic tests? The file attached contains the three (JUnit 4) test classes testing the static methods (as far as I have changed them), if you want to add them to a some kind of test system.

[elias]

I just now noticed that the ReadableVector* types are in fact interfaces. Accessing interface methods is quite a bit slower than accessing classes, even though the methods themselves are abstract (because of the multiple inheritance with interfaces vs. inheritance with abstract classes). I'm not sure if we want/can convert the ReadableVector* types to be abstract classes instead of interfaces, though.

- elias

[Matzon]

I just made some quick benchmarks on this - and it showed a performance drop of about 2.1% (prob +/- 0.5%)

[Matzon]

sorry, that was 21,82% 

public class Benchmark {

	/**
	 * @param args
	 */
	public static void main(String[] args) {
		long time = System.nanoTime();
		 
		Vector3f vector = new Vector3f((float)Math.random(), (float)Math.random(), (float)Math.random());
		for(int j=0; j<1000; j++) {
			float result = 0;
			for(int i=0; i<1000000; i++) {
				result += Vector3f.dot(vector, vector);
				vector.set(result, result, result);
			}
 
			//System.out.println(vector + " dot => " + result);
		}
 
		long time2 = (System.nanoTime() - time);
		System.out.println("Time: " + time2 + ", " + (time2/1000/1000/1000) + "s" );
	}
}

[Matzon]

With patch:
Time: 10157392350, 10s
Time: 10085646360, 10s

Without patch:
Time: 7884583934, 7s
Time: 7884842627, 7s

Java 1.6.0_03, Core 2 Duo E6750@3.2, Windows XP, Client VM

I tried running with Server VM:

Patched:
Time: 6681096061, 6s
Time: 6684694283, 6s

Unpatched:
Time: 6023337883, 6s
Time: 6007725283, 6s

[Jens v.P.]

This is why I asked in my first posting whether it's a design bug or a performance issue... BTW: Did you run the benchmark with j<1000? On my MacBook Pro (C2D 2,2 Ghz, OS X) I got 7(,8) seconds with j<10 and 78 seconds with j<100.

Anyway... I really think that ReadableVector?f should be used instead of Vector?f wherever possible. So I think Elias' idea of making ReadableVector?f an abstract class would solve the problem. The fields may be defined protected, so the static methods could directly access the fields, but they are not visible from outside the class hierarchy.

Luckily, the Vector classes are not derived from each other, so we do not run into problems. IMHO, the following modifications are to be made (R = ReadableVector, V = Vector, W = WriteableVector, R/V/W4 omitted here):

interface Vector                       -- was class
interface W2 extends Vector
interface W3 extends W2

abstract class R extends Vector         -- was interface
abstract class R2 extends R
abstract class R3 extends R2

class V2 extends R2 implements W2   -- was: extends V  ...
class V3 extends R3 implements W3   -- was: extends V  ...

This way, V2 .. V4 can access the fields x, y, z, w directly, from outside the class hierarchy final getters can be used.
The Vector class has gone, method length() is to be implemented in R (where it belongs since it's a read only method).
normalise() has to be declared in the interface now and has to be implemented in V2, V3, and V4 (which is the only trade-off here).

The results returned by "instance of" are not changed except W is now an instance of V (which seems ok since V becomes a kind of W0 interface). Could this cause problems?
The good thing is that V3 is not an instance of V2 in the current version. So we do not run into problems here.

If you do not object I can implement this in the next days and provide another patch here, too.

Jens

[Jens v.P.]

Thinking about it, there's no need to define R, R2, .. abstract. Defining a constructor with parameters enables immutable (i.e. readable) Vector classes.

[elias]

Looks ok, except that the fields are now protected and thus cannot be accessed from the outside directly (they're public correctly). How about keeping the fields in the concrete classes (Vector2f, Vector3f, Vector4f) and just have public abstract getX/Y/Z() in the ReadableVector* classes? Hopefully we keep the performance and keep the ability to access vector*f.x/y/z.

- elias

[Jens v.P.]

If the fields are defined in Vector, the getters are to be declared abstract (and yet virtual) in ReadableVector. If the fields are defined in ReadableVector, the getters can be defined final (and setters in vector, too). From within the Vector hierarchy, direct field access is possible. So, operations requiring many accesses to the fields can be implemented in Vector and descendants, which may be defined by clients, too.

I assume there is a performance ordering:
- direct field access (fastet)
- final methods
- virtual methods (slowest)
We should measure the "distance" between these three methods and decide on the results.

Besides these considerations, another question is how the effect of using an immutable (i.e. readable) class can have a positive effect on the client's design and performance, e.g. by reducing the necessity to create copies. I could imagine that creating a single copy of a vector costs more performance than 20 accesses to virtual methods.

Last but not least: If the readable vectors cannot be used in vector operations, they seem pretty useless to me.

Jens

[princec]

At this stage I would rather worry about correctness of design rather than absolute performance; the hybrid JVM coming next year is effectively going to minimise the differences between server and client compilers.

Cas