- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.1k
          fix(core): use BufferMapState::Active for any BufferUsages::MAP_* flags
          #8426
        
          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
base: trunk
Are you sure you want to change the base?
  
    fix(core): use BufferMapState::Active for any BufferUsages::MAP_* flags
  
  #8426
              Conversation
BufferMapState::Active for any BufferUsages::MAP_* flagsBufferMapState::Active for any BufferUsages::MAP_* flags
      | Hmmm…I'm not immediately sure how to fix the CI errors, which all seem to be on GL backends (incl. WebGL). I'm not familiar with GL backends, so I'm sure I'm doing something wrong with bookkeeping there, but not what that might be. | 
60bd075    to
    10d652f      
    Compare
  
    | Going to mark this as ready for review. I'm not sure how to resolve CI yet, but at least the approach can be validated, and maybe a reviewer will know more than I do about how to resolve this. 😖 | 
10d652f    to
    146f37e      
    Compare
  
    
      
        
              This comment was marked as resolved.
        
        
      
    
  This comment was marked as resolved.
| Hmm, some WebGL tests failed with ac38c82 with recursive snatch log errors, but I'm not sure that they're related to this PR. Dump of the error log:I guess we'll see if they reproduce with 34a3316, which should be the same repository state. EDIT: It does. Ugh. 😩 | 
34a3316    to
    684859e      
    Compare
  
    | I think the WASM/WebGL failure is due to the GL backend assuming it won't see a buffer with  wgpu/wgpu-hal/src/gles/device.rs Lines 529 to 538 in d8f9726 
 This block shouldn't early return if the buffer has  | 
| @cwfitzgerald @magcius This PR adds  | 
| 
 I feel like we should be able to recover the performance here by changing HAL usages after unmapping, but I don't know enough to determine if that's allowed. Is that possible, @teoxoy? | 
ca86fa8    to
    1ee2372      
    Compare
  
    | As long as it's only adding map usages to cpu-resident buffers, which it appears to be, this is totally fine. Buffer usages only matter in so much as they determine which heap to put the buffer on. | 
| 
 Generally you can't change usages once the buffer is created, because they determine what sort of memory the buffer is in. You need to create a new buffer. I guess this is one effect of using the temporary buffer: it decoupled the requirements for initialization from the requirements for use. | 
| 
 Isn't  | 
| I don't quite understand what this is talking about, but it seems like it's using   | 
| 
  | 
| 
 wgpu/wgpu-hal/src/metal/device.rs Line 384 in a682d9e 
 I will defer to people with more experience than I have. But it doesn't seem like  | 
| It sounds like we need to disconnect the buffer usages from the memory heap decision. Hal should expose some way for wgpu-core to tell it about what heap properties it should have, and then core decides based on the user's usages. | 
08562e9    to
    50a7936      
    Compare
  
    | After doing some pairing with @teoxoy and @cwfitzgerald yesterday, we determined that the reason WASM-on-WebGL wasn't working because the emulated mapping workaround in  | 
50a7936    to
    5bdbfea      
    Compare
  
    … flags For cases where a buffer is `mapped_at_creation`, our current implementation of `Buffer::create` initializes the buffer's internal state with `BufferMapState::Init` (which contains a staging buffer underneath the hood) for a descriptor requesting `MAP_READ` that is copied to a host-backed buffer . `MAP_WRITE` works a little differently, starting from the beginning with a host-backed buffer. `Init` does a buffer copy between the staging buffer and the host-backed buffer in the device's queue when the buffer is `unmap`ped. However, `Buffer::map_async` (correctly) assumes that a host-backed buffer need not wait for anything in the queue. This results in a bug where `map_async` doesn't actually wait long enough for the device queue to complete its operations before resolving. Oops! Up to the point where a buffer is unmapped after being mapped at creation, `MAP_READ` and `MAP_WRITE` buffers' capabilities are the same. That is, we should be able to get mutable slices for mapped ranges, no matter what. So, make `MAP_READ` just initialize its internal state in the same way as with `MAP_WRITE`.
5bdbfea    to
    bf25b89      
    Compare
  
    
Connections
MAP_READbuffers that aremapped_at_creation.Description
For cases where a buffer is
mapped_at_creation, our current implementation ofBuffer::createinitializes the buffer's internal state withBufferMapState::Init(which contains a staging buffer underneath the hood) for a descriptor requestingMAP_READthat is copied to a host-backed buffer .MAP_WRITEworks a little differently, starting from the beginning with a host-backed buffer.Initdoes a buffer copy between the staging buffer and the host-backed buffer in the device's queue when the buffer isunmapped. However,Buffer::map_async(correctly) assumes that a host-backed buffer need not wait for anything in the queue. This results in a bug wheremap_asyncdoesn't actually wait long enough for the device queue to complete its operations before resolving. Oops!Up to the point where a buffer is unmapped after being mapped at creation,
MAP_READ,MAP_WRITE, and even non-MAP_*buffers' capabilities are the same. That is, we should be able to get mutable slices for mapped ranges, no matter what. So, makeMAP_READjust initialize its internal state in the same way as withMAP_WRITE.Testing
Added
webgpu:api,operation,buffers,map:mapAsync,read:*, which is expected to fail in the first commit, and is resolved with the second commit.Squash or Rebase?
Rebase.