Skip to content
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

Resizable (jagged) vectors can't have a non-zero starting size #95

Closed
krasznaa opened this issue Aug 26, 2021 · 7 comments · Fixed by #199
Closed

Resizable (jagged) vectors can't have a non-zero starting size #95

krasznaa opened this issue Aug 26, 2021 · 7 comments · Fixed by #199
Assignees

Comments

@krasznaa
Copy link
Member

As I mentioned in #94, this has been bothering me for a while... With a constructor like

    /// Resizable data constructor
    vector_buffer(size_type capacity, size_type size,
                  memory_resource& resource);

, one should assume that the created vector buffer would have a starting size of size. But in the current implementation it doesn't. It always starts from 0. 😦

When you set up a resizable (jagged) vector currently, you do it like:

    vecmem::data::vector_buffer<int> output_buffer(input.size(), 0,
                                                   device_resource);
    m_copy.setup(output_buffer);

Where m_copy is one of the "platform specific" copy objects. The problem here is that the buffer constructor currently has no way of storing the starting size that it received, in the memory that it would allocate with the memory resource that it receives. In case the memory resource allocates non-host-accessible memory, one needs to write to that memory using the appropriate "copy class". But by the time that we use the vecmem::copy::setup(...) function, that original size variable is lost. (The buffer does not store it anywhere.)

There are a couple of ways out of it, but they either require some bigger re-organisation in the code, or a setup that I wouldn't really like. I think the following are all our options here:

  • We add one more variable to the buffer classes, which would hold the "starting size" of the buffer. This variable would then be used by vecmem::copy::setup(...) when initialising the (device) memory. Technically this could work (I think...), but I just really don't like this design. (But maybe you guys could convince me otherwise...)
  • In order to create a resizable (jagged) vector buffer, one would need to provide both a memory resource, and a copy object to its constructor. So that the constructor itself would take care of all of the setup that it needs. I'm leaning towards this option at the moment... (However we might run into problems with circular dependencies on this one, since vecmem::copy already knows about the buffer types...)
  • We create "factory functions" on vecmem::copy for constructing resizable (jagged) vector buffers. This is very similar to option number 2, and would introduce a design that we don't use anywhere else in the project so far.

So... Any objections to having to create resizable buffers with code like:

vecmem::cuda::device_memory_resource resource;
vecmem::cuda::copy copy;

vecmem::data::vector_buffer<int> buffer1(1000, 100, resource, copy);
vecmem::data::jagged_vector_buffer<int> buffer2({1000, 1000, 1000}, {10, 100, 200}, resource, copy);

?

@konradkusiak97, I'm interested in your opinion as well. 😉

@paulgessinger
Copy link
Member

Conceptually, option no. 2 sounds like the most reasonable one, given we can break circular dependencies.

@paulgessinger
Copy link
Member

By the way, are we interested in trying to type-erase the copy objects at all?

@krasznaa
Copy link
Member Author

By the way, are we interested in trying to type-erase the copy objects at all?

I'm not sure what you mean by this... The vecmem::copy class has a bunch of template functions, which is not great, but since all of the vecmem containers are templates, I don't know if we could make these non-templated.

Then, all the classes that derive from vecmem::copy and implement the do_memcpy and do_memset functions, are type erased.

So... what did you have in mind exactly? 😕

@konradkusiak97
Copy link

I'm in favor of the 2nd option as well. Doing copy.setup() each time after creating a buffer anyway seems like it is a part of constructing the object so I think it is a good idea to do it inside the constructor.

I don't have a clear picture in mind of what do you mean by the 3rd option though so feel free to write more details if you want 😉

@paulgessinger
Copy link
Member

So... what did you have in mind exactly? 😕

What I'm referring to is if we go with option 2, the constructor needs to accept any copy object. I had forgotten that vecmem::copy is a base for the copy objects, so it's already type-erased.

@krasznaa
Copy link
Member Author

This #^&*$ thing got closed by #199 automatically. 😦 I should have paid more attention.

@guilhermeAlmeida1, let's discuss here instead of #219. Unfortunately this issue has been with us for a very long time. I'm very open for a good API that would allow us to initialize the jagged vector sizes to something non-zero in a convenient way... 🤔

@krasznaa
Copy link
Member Author

Let's close this one finally. The new API of the buffers does not claim that the starting sizes of resizable buffers could be anything but zero. (One needs to vecmem::copy some payload into a buffer to make it have a different "starting size" for a kernel.) And as we discussed, we really don't foresee a need for anything else anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants