Cache Impeller paths in the DisplayList to amortize conversion #50076
Conversation
impeller/geometry/path.cc
Outdated
| namespace impeller { | ||
|
|
||
| Path::Path() { | ||
| Path::Path() : copy_on_shared_write_data_(new Data()) { |
There was a problem hiding this comment.
The way that Paths are structured today, the mutating methods are only visible to PathBuilder, and the rest of Impeller treats them as immutable. This could be implemented differently: we could have path builder hold the data internally and then construct path with all data in the ctor without hidden mutation.
There was a problem hiding this comment.
What I mean is: I think this code will be much simpler if you treat the shared data as read-only and instead adjust how PathBuilder constructs paths.
There was a problem hiding this comment.
Ah, PathBuilder could hold an instance of the Data struct instead of an instance of Path? Then only it would need a "shared-lock" variable, the Path would be blissfully ignorant of whether its data was being shared with a PathBuilder...?
There was a problem hiding this comment.
I think on construction the PathBuilder would move the data into the Path and then there would never be any need for locking
There was a problem hiding this comment.
I see. It could also trim the data as it moved it which would be a plus.
Right now we don't need another copy to call TakePath() unless the caller keeps using the PathBuilder.
There was a problem hiding this comment.
I guess this is no different than DisplayListBuilder and DisplayList where it trims the storage and hands it off. And, it's way cleaner in the long run as it isolates mutation code explicitly from an immutable representation.
There was a problem hiding this comment.
Done. Path is now truly immutable and cloned data is truncated (naturally by the default copy constructor).
|
Also, it occurs to me that I didn't write any explicit "make sure the data is copied on write correctly" unit tests and I also didn't really look through the existing Path tests to see if there was a test that covered that case. I'll want some more explicit tests on that to be sure... |
impeller/geometry/path.h
Outdated
| // locked. If the PathBuilder is never used again then we never | ||
| // copy the data. If it is used again, then the next call that | ||
| // modifies the data in the prototype_ will discover that it is | ||
| // locked when |GetModifiableData()| is called and the prototype_ | ||
| // will clone its data at that point - leaving all of the Path | ||
| // references that referred to its most recently "taken" version | ||
| // to share the old data. |
There was a problem hiding this comment.
I don't think this is used anymore, right?
flar
force-pushed
the
dl-cached-impeller-paths
branch
from
701dfe2 to
0fe6e45
Compare
…142369) flutter/engine@2687ddb...95e9a15 2024-01-27 [email protected] Cache Impeller paths in the DisplayList to amortize conversion (flutter/engine#50076) 2024-01-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Android] Cache GPU resources using HardwareBuffer's id as key" (flutter/engine#50114) 2024-01-27 [email protected] Roll Dart SDK from 58665e3dee42 to 7ae508ee09a3 (1 revision) (flutter/engine#50112) 2024-01-27 [email protected] Roll Fuchsia Linux SDK from WHlwlOwznFknNm5IS... to GBTh3gOOgmndwT70X... (flutter/engine#50111) 2024-01-27 [email protected] Roll Skia from 6279c88b9e29 to b9b80230c87b (4 revisions) (flutter/engine#50110) 2024-01-27 [email protected] [Impeller] add missing barrier to compute tessellator. (flutter/engine#50108) 2024-01-27 [email protected] [Android] Cache GPU resources using HardwareBuffer's id as key (flutter/engine#50028) 2024-01-27 [email protected] Move Mac builder_cache to prod (flutter/engine#50044) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from WHlwlOwznFkn to GBTh3gOOgmnd If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter#50076)" This reverts commit 95e9a15.
…on (flutter#50076)" This reverts commit 60ce148.
2 participants
DisplayList by default contains Skia paths stored as
SkPathobjects. Impeller must convert these to its internalimpeller::Pathformat on every dispatch of the DisplayList on every frame.This change allows Impeller to store a cached copy of its version of the path into the DisplayList and reuse that instance if it ever encounters the same DisplayList again (likely to happen as most paths come from the Framework which does a lot of work to re-use ui.Picture objects - i.e. DisplayLists).
In order to facilitate this change,
impeller::Pathwas modified to have fast-copy-constructors that share the data and the PathBuilder will copy the mutable data into an immutable version inTakePath().