Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Don't discard GLES buffer for each copy; respect destination offset #33545

Merged
bdero merged 2 commits into from
May 23, 2022
Merged

[Impeller] Don't discard GLES buffer for each copy; respect destination offset #33545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3274bd
Don't discard GLES buffer for each copy; respect destination offset
bdero May 22, 2022
6c71fad
Address comments
bdero May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion impeller/renderer/backend/gles/allocator_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
// found in the LICENSE file.

#include "impeller/renderer/backend/gles/allocator_gles.h"
#include <memory>
Copy link
Member

@chinmaygarde chinmaygarde May 23, 2022

Choose a reason for hiding this comment

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

Nit: Newline between the header types.

Copy link
Member Author

@bdero bdero May 23, 2022

Choose a reason for hiding this comment

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

Done.


#include "impeller/base/allocation.h"
#include "impeller/base/config.h"
#include "impeller/renderer/backend/gles/device_buffer_gles.h"
#include "impeller/renderer/backend/gles/texture_gles.h"
Expand All @@ -24,7 +26,12 @@ bool AllocatorGLES::IsValid() const {
// |Allocator|
std::shared_ptr<DeviceBuffer> AllocatorGLES::CreateBuffer(StorageMode mode,
size_t length) {
return std::make_shared<DeviceBufferGLES>(reactor_, length, mode);
auto buffer = std::make_shared<Allocation>();
if (!buffer->Truncate(length)) {
return nullptr;
}
return std::make_shared<DeviceBufferGLES>(reactor_, std::move(buffer), length,
mode);
}

// |Allocator|
Expand Down
4 changes: 2 additions & 2 deletions impeller/renderer/backend/gles/buffer_bindings_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl,
return false;
}
const auto& device_buffer_gles = DeviceBufferGLES::Cast(*device_buffer);
const uint8_t* buffer_ptr = device_buffer_gles.GetBufferData()->GetMapping() +
buffer.resource.range.offset;
const uint8_t* buffer_ptr =
device_buffer_gles.GetBufferData() + buffer.resource.range.offset;

if (metadata->members.empty()) {
VALIDATION_LOG << "Uniform buffer had no members. This is currently "
Expand Down
37 changes: 23 additions & 14 deletions impeller/renderer/backend/gles/device_buffer_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

#include "impeller/renderer/backend/gles/device_buffer_gles.h"

#include <cstring>
#include <memory>

#include "flutter/fml/trace_event.h"
#include "impeller/base/allocation.h"
#include "impeller/base/config.h"
Expand All @@ -12,12 +15,14 @@
namespace impeller {

DeviceBufferGLES::DeviceBufferGLES(ReactorGLES::Ref reactor,
std::shared_ptr<Allocation> buffer,
size_t size,
StorageMode mode)
: DeviceBuffer(size, mode),
reactor_(std::move(reactor)),
handle_(reactor_ ? reactor_->CreateHandle(HandleType::kBuffer)
: HandleGLES::DeadHandle()) {}
: HandleGLES::DeadHandle()),
buffer_(std::move(buffer)) {}

// |DeviceBuffer|
DeviceBufferGLES::~DeviceBufferGLES() {
Expand All @@ -35,21 +40,23 @@ bool DeviceBufferGLES::CopyHostBuffer(const uint8_t* source,
return false;
}

if (offset + source_range.length > size_) {
// Out of bounds of this buffer.
if (!reactor_) {
return false;
}

if (!reactor_) {
if (offset + source_range.length > size_) {
// Out of bounds of this buffer.
return false;
}

auto mapping =
CreateMappingWithCopy(source + source_range.offset, source_range.length);
if (!mapping) {
if (offset + source_range.length > buffer_->GetLength()) {
return false;
}
data_ = std::move(mapping);

std::memmove(buffer_->GetBuffer() + offset, source + source_range.offset,
Copy link
Member

@chinmaygarde chinmaygarde May 23, 2022

Choose a reason for hiding this comment

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

Oof. Good catch.

source_range.length);
dirty_ = true;

return true;
}

Expand Down Expand Up @@ -78,11 +85,13 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const {

gl.BindBuffer(target_type, buffer.value());

if (!uploaded_) {
if (dirty_) {
TRACE_EVENT0("impeller", "BufferData");
gl.BufferData(target_type, data_->GetSize(), data_->GetMapping(),
gl.BufferData(target_type, buffer_->GetLength(), buffer_->GetBuffer(),
Copy link
Member

@chinmaygarde chinmaygarde May 23, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bdero bdero May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@chinmaygarde chinmaygarde May 23, 2022

Choose a reason for hiding this comment

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

Right. The bug looks good.

GL_STATIC_DRAW);
uploaded_ = true;
dirty_ = false;
has_uploaded_ = true;

reactor_->SetDebugLabel(handle_, label_);
}

Expand All @@ -92,7 +101,7 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const {
// |DeviceBuffer|
bool DeviceBufferGLES::SetLabel(const std::string& label) {
label_ = label;
if (uploaded_) {
if (has_uploaded_) {
reactor_->SetDebugLabel(handle_, label_);
}
return true;
Expand All @@ -105,7 +114,7 @@ bool DeviceBufferGLES::SetLabel(const std::string& label, Range range) {
return SetLabel(label);
}

std::shared_ptr<fml::Mapping> DeviceBufferGLES::GetBufferData() const {
return data_;
const uint8_t* DeviceBufferGLES::GetBufferData() const {
return buffer_->GetBuffer();
}
} // namespace impeller
15 changes: 11 additions & 4 deletions impeller/renderer/backend/gles/device_buffer_gles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@

#pragma once

#include <memory>

#include "flutter/fml/macros.h"
#include "impeller/base/allocation.h"
#include "impeller/base/backend_cast.h"
#include "impeller/renderer/backend/gles/reactor_gles.h"
#include "impeller/renderer/device_buffer.h"
Expand All @@ -15,12 +18,15 @@ class DeviceBufferGLES final
: public DeviceBuffer,
public BackendCast<DeviceBufferGLES, DeviceBuffer> {
public:
DeviceBufferGLES(ReactorGLES::Ref reactor, size_t size, StorageMode mode);
DeviceBufferGLES(ReactorGLES::Ref reactor,
std::shared_ptr<Allocation> buffer,
size_t size,
StorageMode mode);

// |DeviceBuffer|
~DeviceBufferGLES() override;

std::shared_ptr<fml::Mapping> GetBufferData() const;
const uint8_t* GetBufferData() const;

enum class BindingType {
kArrayBuffer,
Expand All @@ -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_;
Copy link
Member

@chinmaygarde chinmaygarde May 23, 2022

Choose a reason for hiding this comment

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

I think a better name for this would be backing_store_.

Copy link
Member Author

@bdero bdero May 23, 2022

Choose a reason for hiding this comment

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

Done.

mutable bool dirty_ = false;
Copy link
Member

@chinmaygarde chinmaygarde May 23, 2022

Choose a reason for hiding this comment

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

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_.

Copy link
Member Author

@bdero bdero May 23, 2022

Choose a reason for hiding this comment

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

Great idea, done.

mutable bool has_uploaded_ = false;

// |DeviceBuffer|
bool CopyHostBuffer(const uint8_t* source,
Expand Down