Some general Vector3 optimizations

[spydon]

Since the Vector3.zero constructor was already a normal named constructor it was already possible to extend the class.

• Converts all factory constructors (except Vector3.random) to named constructors that directly sets the storage instead of calling intermediary methods to do it.
• Simplifies length2, dot, isInfinite and isNaN.
• Optimizes reflect, relativeError and absoluteError so that they don't create extra Vector3 objects.
• Orders all operations made on _v3storage from 1 to 0 so that it is consistent across the class and guarantees that bounds checking is only done once.

[coveralls]

Coverage: 26.315% (-23.1%) from 49.455% when pulling f4a067a on spydon:fix/vector3 into e3de8da on google:master.

[spydon]

@johnmccutchan fixed up Vector3 here, I'll hopefully have time to fix Vector4 tomorrow.

[rakudrama]

Since the Vector3.zero constructor was already a normal named constructor it was already possible to extend the class.

I think we will want to seal this class to prevent it being extended. This will make performance and behaviour more reliable.

The classes in vector_math were never designed to be extended. There are no guarantees which methods are implemented in terms of other methods to provide an extension point. When Vector3 is extended, most of the behaviour is working by accident rather than by design. If Vector3 was designed to be extended, this would be documented. Say it was the intention to allow a subclass to implement some kind of observable Vector3. Then all mutating methods would have been implemented in terms of setValue so that setValue could be overridden as a single place to detect changes.

Dart 3 now has class modifiers to prevent classes being extended or implemented. When vector_math is revised to be a Dart 3 library, the ability to extend Vector3 should be removed.

The efficiency concern is that extending a class makes calls to the base class methods be polymorphic. When the class is not extended, calls to a method can be resolved exactly and inlined or implemented as direct calls. When the methods have overrides, the calls are necessarily indirect calls as there may be more than one version of the method. For a simple class like Vector3 this can be a significant performance penalty. Say there is an app that uses Vector3 and is fast. One day the app is changed to import a library that extends Vector3. All the existing code becomes slower because of the calls that become indirect (and can no longer be optimized).

• Converts all factory constructors (except Vector3.random) to named constructors that directly sets the storage instead of calling intermediary methods to do it.

I worry that these changes may confuse the (somewhat brittle) analysis that allow the length of _v3storage to sometimes become known in the optimizer.

• Simplifies length2, dot, isInfinite and isNaN.
• Optimizes reflect, relativeError and absoluteError so that they don't create extra Vector3 objects.
• Orders all operations made on _v3storage from 1 to 0 so that it is consistent across the class and guarantees that bounds checking is only done once.

These all seem like nice changes.

[spydon]

I think we will want to seal this class to prevent it being extended. This will make performance and behaviour more reliable.

Since that will be a big breaking change, I'm guessing you won't do that any time soon when looking at how this package has been developed previously? And therefore this change shouldn't make any difference meanwhile?

If Vector3 was designed to be extended, this would be documented.

There is no documentation about that it shouldn't be extended either, and since it also hasn't been marked with the sealed meta tag that have existed for quite a while I think many of the users of this package expect that there isn't any limitation on extending it.

I worry that these changes may confuse the (somewhat brittle) analysis that allow the length of _v3storage to sometimes become known in the optimizer.

There really should be some form of benchmark suite in this repository if it is that focused on performance, because these kind of claims are very hard to check otherwise.

These all seem like nice changes.

Thanks!

[johnmccutchan]

While I agree with the spirit of your suggestion @rakudrama I feel like this is likely to break a number of users of this library? How should we proceed?

[johnmccutchan]

Actually, let's have that discussion in a separate issue: #298

[johnmccutchan]

@rakudrama what changes in this PR do you consider blocking? I'd like to split them off and land the non-controversial ones.

[rakudrama]

@rakudrama what changes in this PR do you consider blocking? I'd like to split them off and land the non-controversial ones.

I'm ok with everything except changes to the constructors (and anything that changes a public interface but as far as I remember, we fixed those).
We can make progress by landing the non-constructor changes.

I am looking at the benchmark that was written to support changing the constructors.
Unfortunately, the compilers optimize a lot of the benchmark away, so it does not really make the argument.
It is instead a test of how good the compiler is as removing unused computations.
I will make comments there to make the benchmark better.

I understand that this CL is just following what was done and approved for Vector2.
It is difficult to keep the library cohesive without a design process to tie together multiple CLs.