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

Buffer mapping API #377

Merged
merged 13 commits into from
Oct 17, 2023
Merged

Buffer mapping API #377

merged 13 commits into from
Oct 17, 2023

Conversation

almarklein
Copy link
Member

@almarklein almarklein commented Oct 15, 2023

This closes #114, also updated that issue's top comment to explain the issue better.

Repeating the advantages of the WebGPU mapping API for clarity:

  • Allows reading and writing data in a buffer.
  • Allows doing that with subranges.
  • Allows doing that with multiple subranges in one go, i.e. without mapping/unmapping multiple times.
  • Without unnecessary data copies.

I've had another look at disabling the base array so that the views don't work anymore, but without success.

Therefore this proposal does not expose a mapped array to read from / write to. Instead it has two methods for this: read_mapped() and write_mapped(). The former has a kwarg copy that can be set to False to get the actually mapped data. Users who know what they're doing can then benefit from more performance.

With this new API we support all the four points above. It's also a lot closer to the WebGPU spec: we have map() and unmap() in between which the data can be read or written multiple times (fast!). But instead of obtaining a piece of the mapped memory (getMappedRange()), we have read_mapped() and write_mapped(), so we don't have to expose the mapped memory (except whith read_mapped(..., copy=False).

@almarklein almarklein changed the title I tried many ways, thought I found one, but did not. Buffer mapping API Oct 15, 2023
@almarklein
Copy link
Member Author

I moved the test file with cases that do not work to a Gist: https://gist.github.com/almarklein/a7b59d5cf18ae979f79602115807b0b2

@Korijn
Copy link
Collaborator

Korijn commented Oct 16, 2023

This may be ignorant but how bad is it really that you could have a view on released memory? It could just be a documented risk that users of wgpu-py need to be aware of.

@almarklein
Copy link
Member Author

Well we could document it carefully, but it's so easy to forget, also because this is not something people are used to having to care about in Python. What makes it worse is that these views can easily end up in a different place unrelated to wgpu. That, plus the fact that segfaults are sometimes not reproducible (sometimes it will crash, sometimes you get bogus data), can result in some nasty bug tracking ...

I also realized just now that the data-copy downside of only an issue when reading from the buffer. When writing it's not because Python/numpy makes use of views so much.

@almarklein
Copy link
Member Author

Just pushed a proposal for an API. The buffer can be mapped and unmapped like the WebGPU spec, but instead of requesting an array-like object (that represents the mapped memory) to read from or write to, the buffer has methods read_mapped() and write_mapped().

Still have to adjust examples and tests.

wgpu/backends/rs.py Outdated Show resolved Hide resolved
@Korijn
Copy link
Collaborator

Korijn commented Oct 16, 2023

Is it guaranteed that the buffer will not be unmapped until you call the unmap function?

If so, a context manager might be very nice in combination with the proposed copy=True kwarg, to create a "safe space" where you can work with a zero-copy guaranteed-mapped buffer?

with buffer.map(copy=False) as view:
    view.do_stuff()  # safe?

@almarklein
Copy link
Member Author

Is it guaranteed that the buffer will not be unmapped until you call the unmap function?

The other way is that the buffer gets destroyed.

@almarklein
Copy link
Member Author

Ran into an interesting edge-case. Filed an issue upstream: gfx-rs/wgpu-native#305 For now I fixed it here.

@almarklein
Copy link
Member Author

This change does not actually require changes in PyGfx - it looks like we only used the parts of the API that have not changed.

@almarklein almarklein marked this pull request as ready for review October 17, 2023 12:16
@almarklein almarklein merged commit d7b17e2 into main Oct 17, 2023
18 checks passed
@almarklein almarklein deleted the buffermapping branch October 17, 2023 12:40
@almarklein
Copy link
Member Author

This change does not actually require changes in PyGfx - it looks like we only used the parts of the API that have not changed.

Woops, using it in the picking logic. Will prepare a pr for pygfx shortly.

This was referenced Oct 20, 2023
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 this pull request may close these issues.

Making buffer mapping part of the public API?
2 participants