flame icon indicating copy to clipboard operation
flame copied to clipboard

Pool for `Vector2`

Open spydon opened this issue 3 years ago • 6 comments

Problem to solve

Since we are creating many Vector2 objects in the code base our performance would benefit from having a pool for Vector2.

Proposal

The syntax should be non-intrusive, possibly an extension to Vector2.

More information

Are you interested in working on a PR for this? Yes,

spydon avatar Apr 10 '22 12:04 spydon

I wonder how can the Vector2 objects be returned to the pool? Would we need to explicitly dispose() of them?

st-pasha avatar Apr 10 '22 19:04 st-pasha

I wonder how can the Vector2 objects be returned to the pool? Would we need to explicitly dispose() of them?

Yeah, I don't think there is a way around that.

I just remembered that static methods and factories in extensions can't be used directly on the underlying object. I had thought of having something like Vector2.get/new to get a vector from the pool, but maybe extensions isn't the best way to go then, or having an extension named `Vector2P or something.

spydon avatar Apr 10 '22 19:04 spydon

So, let's take for example a function like this:

Vector2 get center => (start + end)..scaled(0.5);

This creates a Vector2 object, so it's an obvious client for using the vector pool. I would imagine it would look something like this:

Vector2 get center => VectorPool.get()..setFrom(start)..add(end)..scaled(0.5);

A bit less readable, but we can tolerate that.

Now, the vector returned by this method is a "pool" vector, so the user must now:

  • be aware that this vector is from a pool;
  • explicitly call VectorPool.dispose(center) at the end of the vector's lifecycle.

I don't think this part of the contract is feasible...

st-pasha avatar Apr 10 '22 22:04 st-pasha

I would fo something like:

Vector2 get center => VectorPool.getFrom(start)..add(end)..scaled(0.5);

And then:

center.dispose();

Possibly, we could also offer a set of numbered temporary vectors inside the pool so that it would be like:

Vector2 get center => VectorPool.get(0)..setFrom(start)..add(end)..scaled(0.5);

And then you don't have to call dispose in the end of your method because you are responsible for the contract that the same indexed vector shouldn't be reused within its scope.

Any other ideas?

spydon avatar Apr 11 '22 06:04 spydon

Disposing is problematic.

There are lots of vector operations going around, some of them performed by the vector_math library, some by flame engine, some by the user. Which means that no matter how you try, there will always be some non-pool vectors going around. Which means that:

  • for every operation returning a vector, we need to clearly document whether it's a pool vector or "regular" one;
  • the user needs to carefully check documentation for every such method -- there is no indication in the method name or signature that it returns a pool vector;
  • the user then needs to remember to dispose of any pool vector that they receive. This would be difficult and error-prone, with no help from the compiler.
    • many vectors are used on-the-fly: center + size/2 -- now you'd need to store all these vectors into temporary variables, making code much more cumbersome.
    • sometimes the logic requires that you mix regular and pool vectors: point = condition? center : position, where center is pooled and position is a regular vector. Now point is a Schrodinger cat: maybe you need to dispose it, or maybe you don't.

In short, I'm worried that the API where some (or all) of the vectors need to be explicitly disposed, will lead to a sever degradation of user experience, and/or massive memory leaks that are difficult to debug.

In addition, the administration of the pool itself (acquiring/returning objects) might bring additional expense that could overcome any savings from not creating extra objects.

So I would recommend, before committing to this functionality, to get a clear understanding of the following:

  • what is the actual efficiency loss in creating extra vector objects. For example, how does the timing of (a+b)/2 compares to a.clone()..add(b)..scaled(1/2)?
  • how do vector objects compare to Offset objects? For example, if a and b in the above were Offsets, would they be faster/slower?
  • what would be the performance when using a simple implementation of a vector pool (preferably, test under the scenario when the result is not disposed of immediately)?
  • what would be the performance of a custom class MyVector { double x,y; } implementation?
  • what about the NotifyingVector2s that we use in PositionComponents (and hence almost everywhere)?

In addition to simple arithmetics, another very common operation that we use all the time is transformation by the Transform2D matrix. So we might want to do the comparison for this operation as well.

st-pasha avatar Apr 11 '22 18:04 st-pasha

* for every operation returning a vector, we need to clearly document whether it's a pool vector or "regular" one;

I don't think we should ever return a pool vector to the user. Not with this type of API at least.

  * many vectors are used on-the-fly: `center + size/2` -- now you'd need to store all these vectors into temporary variables, making code much more cumbersome.

We already try to do that today, the pool API shouldn't make it more cumbersome than it already is.

  * sometimes the logic requires that you mix regular and pool vectors: `point = condition? center : position`, where `center` is pooled and `position` is a regular vector. Now `point` is a Schrodinger cat: maybe you need to dispose it, or maybe you don't.

In these cases, if we'd want to use a pool vector I think it should be done explicitly. Something like point = (condition? center : position).pooled` maybe, but I don't think this would happen often because the pooled vectors wont come from fields and public methods.

In short, I'm worried that the API where some (or all) of the vectors need to be explicitly disposed, will lead to a sever degradation of user experience, and/or massive memory leaks that are difficult to debug.

Just to repeat what I said earlier, they shouldn't be exposed.

Agree that we should do some benchmarking so that we have some data to base this decision on.

spydon avatar Apr 11 '22 18:04 spydon