[Impeller] Don't discard GLES buffer for each copy; respect destination offset #33545
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -3,7 +3,9 @@ | |||
| // found in the LICENSE file. | |||
|
|
|||
| #include "impeller/renderer/backend/gles/allocator_gles.h" | |||
| #include <memory> | |||
There was a problem hiding this comment.
Nit: Newline between the header types.
| return false; | ||
| } | ||
| data_ = std::move(mapping); | ||
|
|
||
| std::memmove(buffer_->GetBuffer() + offset, source + source_range.offset, |
| @@ -33,8 +39,9 @@ class DeviceBufferGLES final | |||
| ReactorGLES::Ref reactor_; | |||
| HandleGLES handle_; | |||
| std::string label_; | |||
| mutable std::shared_ptr<fml::Mapping> data_; | |||
| mutable bool uploaded_ = false; | |||
| mutable std::shared_ptr<Allocation> buffer_; | |||
There was a problem hiding this comment.
I think a better name for this would be backing_store_.
| mutable std::shared_ptr<fml::Mapping> data_; | ||
| mutable bool uploaded_ = false; | ||
| mutable std::shared_ptr<Allocation> buffer_; | ||
| mutable bool dirty_ = false; |
There was a problem hiding this comment.
IMO, the dirty_ and has_uploaded_ is becoming hard to reason about. The only reason has_uploaded_ exists is because debug object labels cannot be set be on textures and buffers that are generated but not bound (they are so called "create on bind" resources). Unfortunately, glCreateBuffers and friends are not available on OpenGL ES 2. So the has_uploaded_ field was tracking the create (via bind and data upload).
Perhaps you can add a field here that bumps up the generation of data whenever we get a SetContents call and another field that notes the generation that's been uploaded (so, generation_ and uploaded_generation_). We can set debug labels and such when uploaded_generation > 0 and upload data when uploaded_generation_ != generation_.
| TRACE_EVENT0("impeller", "BufferData"); | ||
| gl.BufferData(target_type, data_->GetSize(), data_->GetMapping(), | ||
| gl.BufferData(target_type, buffer_->GetLength(), buffer_->GetBuffer(), |
There was a problem hiding this comment.
I realize this is a bit yak-shavy, but there is a pessimization here where the entire arena will get uploaded even when only a sub-range has been modified.
In a later patch, we could setup the backing store to use a sparse mmap'ed arena and memmove into it any data the application provides. Then, note the range that must be flushed. During the upload, use glSubTexImage2D to upload the arenas that need to be flushed (after consolidation for overlapping ranges and such).
It is just the tedium of dealing with GLES 2.0 :/
A yak-shave within a yak-shave: impeller/base needs an mmaped allocation for sparse arenas. We just never ended up needing these in the Metal backend (and will probably not need them for anything other than ES). The posix stuff ought to be easy. The windows impl may need someone with a Windows box.
There was a problem hiding this comment.
Oh yes, for sure. I added a bug here. Did you mean glBufferSubData (as opposed to glSubTexImage2D)? Although having sparse texture uploading would be good too.
Having the backing store be an mmapped arena sounds like a good idea -- just to make sure we're on the same page: The aim would be to make the arena contents have a per-upload lifetime (flush), that way we only ever hang onto enough memory to keep track of what needs uploading, and avoid needlessly mirroring all the device data.
There was a problem hiding this comment.
Right. The bug looks good.
…estination offset (flutter/engine#33545)
Ran into this while working on impeller-cmake-example. The ImGui Impeller backend allocates one geometry buffer per frame, performing two contiguous buffer copies using
DeviceBuffer::CopyHostBuffer: One for vertices and one for indices.This fixes two problems:
CopyHostBuffercall.Made lots of progress with impeller-cmake on Windows this weekend! Screenshot of the example so far: