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

Implement GPUBuffer.getMappedRange() #27126

Merged
merged 2 commits into from Jul 1, 2020

Conversation

kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jun 30, 2020

The new fields added to GPUBuffer are-

  1. mapping- This is the buffer data. This is not directly exposed to user and is refcounted to keep track of the number of ArrayBuffers that point to the content in it.
  2. mapping_range- The range of mapped portion of the GPUBuffer.
  3. mapped_ranges- Ranges that the various ArrayBuffers expose to the user. Used for validation of getMappedRange()
  4. js_buffers- Actual ArrayBuffers that expose the data to the user. They are detached on unmap.
  5. map_promise- Promise that represents the pending mapping.

This PR also changes the order of the exit of threads. WebGL thread is responsible for sending Exit message to WebRender and therefore should be exited after WebGPU threads.

r?@kvark @jdm


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

@highfive
Copy link

highfive commented Jun 30, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/gpubuffer.rs, components/script/dom/webidls/GPUBuffer.webidl, components/script/dom/gpudevice.rs, components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @KiChjang: components/script/dom/gpubuffer.rs, components/script/dom/webidls/GPUBuffer.webidl, components/script/dom/gpudevice.rs

@highfive
Copy link

highfive commented Jun 30, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@kunalmohan kunalmohan added this to In progress in WebGPU MVP via automation Jun 30, 2020
Copy link
Member

@kvark kvark left a comment

Looks reasonable overall! Noted a few small things

components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
kvark
kvark approved these changes Jul 1, 2020
Copy link
Member

@kvark kvark left a comment

Thank you, looks great!

(m_end - offset) as usize,
m_info.mapping.borrow_mut()[offset as usize..m_end as usize].as_mut_ptr() as _,
Some(free_func),
Rc::into_raw(m_info.mapping.clone()) as _,
Copy link
Member

@kvark kvark Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at this piece a bit. So this would keep the memory for the ArrayBuffer around until it's GC-ed.
I think it's totally fine to land in this stage. Just wanted to note that for the end game, we probably want to "detach" the array buffer on unmap(), so that any access to its contents would be invalid and trigger an exception. Perhaps, we could leave a comment somewhere about this TODO

Copy link
Collaborator Author

@kunalmohan kunalmohan Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already detach the actual array buffers(which are the only ones exposed to the user) on unmap. We also drop the Rc pointer for this array buffer in free_func and set the map_info to None (so we drop all Rc pointers on unmap). What else are we missing here? Should the contents be set to ptr::null() in free_func?

@kvark
Copy link
Member

kvark commented Jul 1, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2020

📌 Commit f96cfaa has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2020

Testing commit f96cfaa with merge 46a469d...

bors-servo added a commit that referenced this issue Jul 1, 2020
Implement GPUBuffer.getMappedRange()

<!-- Please describe your changes on the following line: -->
The new fields added to `GPUBuffer` are-
1. `mapping`- This is the buffer data. This is not directly exposed to user and is refcounted to keep track of the number of `ArrayBuffer`s that point to the content in it.
2. `mapping_range`- The range of mapped portion of the GPUBuffer.
3. `mapped_ranges`- Ranges that the various `ArrayBuffer`s expose to the user. Used for validation of `getMappedRange()`
4. `js_buffers`- Actual `ArrayBuffer`s that expose the data to the user. They are detached on unmap.
5. `map_promise`- Promise that represents the pending mapping.

This PR also changes the order of the exit of threads. WebGL thread is responsible for sending `Exit` message to WebRender and therefore should be exited after WebGPU threads.

r?@kvark @jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2020

💔 Test failed - status-taskcluster

@kvark
Copy link
Member

kvark commented Jul 1, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2020

📌 Commit 891a3bd has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2020

Testing commit 891a3bd with merge 8713954...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2020

☀️ Test successful - status-taskcluster
Approved by: kvark
Pushing 8713954 to master...

@bors-servo bors-servo merged commit 8713954 into servo:master Jul 1, 2020
2 checks passed
WebGPU MVP automation moved this from In progress to Done Jul 1, 2020
@kunalmohan kunalmohan deleted the gpu-mapped-range branch Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
WebGPU MVP
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants