-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Perhaps switch to pointers for matrices and vectors #29
Comments
Another snag with wholesale switching to pointers is the Quaternion type. Right now it's
Should this be changed to
or kept the same and have a pointer to the Quat struct (assuming we go with option 1 above)? |
I was gonna comment on this, but I forgot what I wanted to say. Honestly, I think it's a hard question and I don't know what's the best thing to do. |
(i initially brought up the problem)
multiplication by reference: 11% faster "The long and short is: once you get to Mat4 size, it is a significant speed improvement (about 13 ns/op for multiplication)" @Jragonmiris you can find the implementation of the benchmark here as for wether or not to break the api, the library seems to be 6 month old (according to first commit), might as well make it right today then to pay for it later. There's only so much you can break. Also, since the code is mostly generated, you could offer the new (pointer and dst) version as the main version and a deprecated version with the pass-by-value, something like
for the quaternions, the right answer is "whatever is fastest" but i suspect it's the 2nd option (with the pointer) because quaternion multiplication probably suffers a lot from pass-by-value
|
If you're going to do that, you might as well use gopkg.in with semantic versioning. I have to admit, it's starting to grow on me because it allows one to improve APIs with less fear of breaking dependent code. |
I've done some benchmarking, though with With pointers, a destination, and SIMD, performing element-wise multiplications of two float32 vectors is only barely slower than multiplying two float32s. Once pointers come in, it gets tricky: Two float32s: 0.5ns/op Reallocating a new dst is over 25x slower than having a preallocated dst. More importantly, it's about 4x slower than what we have now. What do I mean by "reallocating a new dst"? I mean, essentially, this:
That single step of creating that new Vec4 single handedly wreaks havoc performance. It seems irresponsible to have a function like that, since people will likely use it due to the much nicer syntax than a Thoughts? |
well according to those results, keep returning by value and/or allow a dst function on top, this way people can chose, I know my app takes care of creating mat4 in advance. this way if people don't wish to care about performance, they can still use the old non-dst version.
|
I think the direction to go for the time being (ignoring any theoretical code changes we can do due to things possibly happening in December with this package), would be to change all functions to have a pointer receiver, this is possible and will result in minimal code breakage due to auto-addressing. Functions will still take vectors as arguments and return vectors. MulIn functions will be added, e.g.
|
holy crap these results are wrong, what happens is cpu cache kicks in and makes pointer faster then they really are, memory access is super expensive. I don't know what exactly the right solution is, I just know these results shouldn't be considered |
As long as Ident4() returns a unique copy, not a pointer to a globally defined variable, this will continue to work fine. Indexing, as well as the built-in What would break are conversions between mathgl types and arrays or f32/f64 types. |
I think the package has been stable for too long to risk a breaking change this big now. |
I got an email recently with some bechmarks. The long and short is: once you get to Mat4 size, it is a significant speed improvement (about 13 ns/op for multiplcation) to use a pointer for Mat4. This is a significant API change and would break a lot of existing code. Doing
Would break. Any code that currently uses any of:
At
andSet
for matrices orX
,Y
,Z
, orW
for vectors is safe (SetX
and the like would likely needed to be added for Vectors).The way I see it, there are two potential solutions here:
func (m *Mat4) MulTo(dst *Mat4, m2 *Mat4)
which would have similar semantics to*MatMN.MulMN(dst, m2)
.I like option 2, since there's no compatability breakage, but it does clutter the API and people may mistakenly use the slower code without realizing it.
Thoughts?
The text was updated successfully, but these errors were encountered: