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

Revert "Fixed constness of display list storage." #52724

Closed
wants to merge 1 commit into from
Closed

Revert "Fixed constness of display list storage." #52724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 14 additions & 14 deletions display_list/display_list.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage,
rtree_(std::move(rtree)) {}

DisplayList::~DisplayList() {
const uint8_t* ptr = storage_.get();
uint8_t* ptr = storage_.get();
DisposeOps(ptr, ptr + byte_count_);
}

Expand Down Expand Up @@ -142,7 +142,7 @@ class VectorCuller final : public Culler {
};

void DisplayList::Dispatch(DlOpReceiver& receiver) const {
const uint8_t* ptr = storage_.get();
uint8_t* ptr = storage_.get();
Dispatch(receiver, ptr, ptr + byte_count_, NopCuller::instance);
}

Expand All @@ -162,16 +162,16 @@ void DisplayList::Dispatch(DlOpReceiver& receiver,
}
const DlRTree* rtree = this->rtree().get();
FML_DCHECK(rtree != nullptr);
const uint8_t* ptr = storage_.get();
uint8_t* ptr = storage_.get();
std::vector<int> rect_indices;
rtree->search(cull_rect, &rect_indices);
VectorCuller culler(rtree, rect_indices);
Dispatch(receiver, ptr, ptr + byte_count_, culler);
}

void DisplayList::Dispatch(DlOpReceiver& receiver,
const uint8_t* ptr,
const uint8_t* end,
uint8_t* ptr,
uint8_t* end,
Culler& culler) const {
DispatchContext context = {
.receiver = receiver,
Expand Down Expand Up @@ -207,7 +207,7 @@ void DisplayList::Dispatch(DlOpReceiver& receiver,
}
}

void DisplayList::DisposeOps(const uint8_t* ptr, const uint8_t* end) {
void DisplayList::DisposeOps(uint8_t* ptr, uint8_t* end) {
while (ptr < end) {
auto op = reinterpret_cast<const DLOp*>(ptr);
ptr += op->size;
Expand All @@ -234,15 +234,15 @@ void DisplayList::DisposeOps(const uint8_t* ptr, const uint8_t* end) {
}
}

static bool CompareOps(const uint8_t* ptrA,
const uint8_t* endA,
const uint8_t* ptrB,
const uint8_t* endB) {
static bool CompareOps(uint8_t* ptrA,
uint8_t* endA,
uint8_t* ptrB,
uint8_t* endB) {
// These conditions are checked by the caller...
FML_DCHECK((endA - ptrA) == (endB - ptrB));
FML_DCHECK(ptrA != ptrB);
const uint8_t* bulk_start_a = ptrA;
const uint8_t* bulk_start_b = ptrB;
uint8_t* bulk_start_a = ptrA;
uint8_t* bulk_start_b = ptrB;
while (ptrA < endA && ptrB < endB) {
auto opA = reinterpret_cast<const DLOp*>(ptrA);
auto opB = reinterpret_cast<const DLOp*>(ptrB);
Expand Down Expand Up @@ -310,8 +310,8 @@ bool DisplayList::Equals(const DisplayList* other) const {
if (byte_count_ != other->byte_count_ || op_count_ != other->op_count_) {
return false;
}
const uint8_t* ptr = storage_.get();
const uint8_t* o_ptr = other->storage_.get();
uint8_t* ptr = storage_.get();
uint8_t* o_ptr = other->storage_.get();
if (ptr == o_ptr) {
return true;
}
Expand Down
12 changes: 4 additions & 8 deletions display_list/display_list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,7 @@ class DisplayListStorage {
DisplayListStorage() = default;
DisplayListStorage(DisplayListStorage&&) = default;

uint8_t* get() { return ptr_.get(); }

const uint8_t* get() const { return ptr_.get(); }
uint8_t* get() const { return ptr_.get(); }

void realloc(size_t count) {
ptr_.reset(static_cast<uint8_t*>(std::realloc(ptr_.release(), count)));
Expand Down Expand Up @@ -311,8 +309,6 @@ class DisplayList : public SkRefCnt {
return modifies_transparent_black_;
}

const DisplayListStorage& GetStorage() const { return storage_; }

Comment on lines -314 to -315
Copy link
Member

Choose a reason for hiding this comment

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

I think the crux of your argument is around exposing this immutable data ( #52705 (comment) ). That decision is tied to #52715 since it accesses it.

Everything else in this PR is correct since it statically asserts correct constness, so you can make the revert just the removal of this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is nothing wrong with the current implementation. Adding const in a few places is probably fine, but not needed since the storage is not exposed outside of the internal implementation.

Two questions I have is whether or not destructors are required to work on const objects (I supposed that they are or the PR would not have landed) and whether or not the internal const-ness (which isn't strictly necessary) would imply that the data in the buffer never changes (but it actually does - there are mutable fields in the data that provided required functions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, yes, the rest of it doesn't hurt anything and might be considered cleaner other than the fact that the data stream actually contains at least one (and potentially more as we migrate the text stuff to be Skia/Impeller-agnostic) mutable chunk of data.

Copy link
Member

Choose a reason for hiding this comment

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

If we need mutable storage in DisplayList the correct solution would to make DisplayList::storage_ not const. Previously it was declared const but had the ability to mutate it because there was a const method that returned a non-const pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't matter as the mutation happens on a const object contained within the DL that is passed to a Dispatch receiver - and it's the receiver that does the mutation.

My point was less "how are we going to do this" than "we do this and making this pointer const won't stop it, so are we sending the right message by making it const", but I think it is a minor point that doesn't matter...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that the field is marked "mutable" already acknowledges that it is allowing mutation inside what is otherwise a const set of data, so I think the point is moot.

private:
DisplayList(DisplayListStorage&& ptr,
size_t byte_count,
Expand All @@ -328,7 +324,7 @@ class DisplayList : public SkRefCnt {

static uint32_t next_unique_id();

static void DisposeOps(const uint8_t* ptr, const uint8_t* end);
static void DisposeOps(uint8_t* ptr, uint8_t* end);

const DisplayListStorage storage_;
const size_t byte_count_;
Expand All @@ -349,8 +345,8 @@ class DisplayList : public SkRefCnt {
const sk_sp<const DlRTree> rtree_;

void Dispatch(DlOpReceiver& ctx,
const uint8_t* ptr,
const uint8_t* end,
uint8_t* ptr,
uint8_t* end,
Culler& culler) const;

friend class DisplayListBuilder;
Expand Down