-
Notifications
You must be signed in to change notification settings - Fork 6k
Hint freed #19842
Hint freed #19842
Changes from all commits
bc9f524
babe728
b351806
0f5ff33
d921c88
b05fac3
d6c50a1
21a9cd9
5bc0710
bd32627
cd42842
4268cfa
3684db4
d506040
3094415
5234e69
6a1a628
d743620
13efdb9
478d773
0fb859c
e4be6ec
3207082
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,11 @@ | |
namespace flutter { | ||
namespace testing { | ||
|
||
class LayerTreeTest : public CanvasTest { | ||
class LayerTreeTest : public CanvasTest, public CompositorContext::Delegate { | ||
public: | ||
LayerTreeTest() | ||
: layer_tree_(SkISize::Make(64, 64), 1.0f), | ||
compositor_context_(fml::kDefaultFrameBudget), | ||
compositor_context_(*this), | ||
root_transform_(SkMatrix::Translate(1.0f, 1.0f)), | ||
scoped_frame_(compositor_context_.AcquireFrame(nullptr, | ||
&mock_canvas(), | ||
|
@@ -33,11 +33,24 @@ class LayerTreeTest : public CanvasTest { | |
CompositorContext::ScopedFrame& frame() { return *scoped_frame_.get(); } | ||
const SkMatrix& root_transform() { return root_transform_; } | ||
|
||
// |CompositorContext::Delegate| | ||
void OnCompositorEndFrame(size_t freed_hint) override { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. // |CompositorContext::Delegate| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done and done |
||
last_freed_hint_ = freed_hint; | ||
} | ||
|
||
// |CompositorContext::Delegate| | ||
fml::Milliseconds GetFrameBudget() override { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
return fml::kDefaultFrameBudget; | ||
} | ||
|
||
size_t last_freed_hint() { return last_freed_hint_; } | ||
|
||
private: | ||
LayerTree layer_tree_; | ||
CompositorContext compositor_context_; | ||
SkMatrix root_transform_; | ||
std::unique_ptr<CompositorContext::ScopedFrame> scoped_frame_; | ||
size_t last_freed_hint_ = 0; | ||
}; | ||
|
||
TEST_F(LayerTreeTest, PaintingEmptyLayerDies) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,7 +137,8 @@ class RasterCache { | |
const SkMatrix& transformation_matrix, | ||
SkColorSpace* dst_color_space, | ||
bool is_complex, | ||
bool will_change); | ||
bool will_change, | ||
size_t external_size = 0); | ||
|
||
void Prepare(PrerollContext* context, Layer* layer, const SkMatrix& ctm); | ||
|
||
|
@@ -156,7 +157,8 @@ class RasterCache { | |
SkCanvas& canvas, | ||
SkPaint* paint = nullptr) const; | ||
|
||
void SweepAfterFrame(); | ||
/// Returns the amount of external bytes freed by the sweep. | ||
size_t SweepAfterFrame(); | ||
|
||
void Clear(); | ||
|
||
|
@@ -192,24 +194,28 @@ class RasterCache { | |
struct Entry { | ||
bool used_this_frame = false; | ||
size_t access_count = 0; | ||
size_t external_size = 0; | ||
std::unique_ptr<RasterCacheResult> image; | ||
}; | ||
|
||
template <class Cache> | ||
static void SweepOneCacheAfterFrame(Cache& cache) { | ||
static size_t SweepOneCacheAfterFrame(Cache& cache) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to document what size is returned from this function. Currently, it seems to only return the layer external size, and one might ask why not also include the image size of the raster cache. Is it because we only want to report the memory size that's managed by Dart VM so the SkImage memory size is irrelevant? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The image size is included in the layer external size. The SkImage size is really of primary importance here. However, we have to figure out how to get that out of what the Dart VM sees - @jason-simmons has a patch open to remove that, but we'd still want it here. |
||
std::vector<typename Cache::iterator> dead; | ||
size_t removed_size = 0; | ||
|
||
for (auto it = cache.begin(); it != cache.end(); ++it) { | ||
Entry& entry = it->second; | ||
if (!entry.used_this_frame) { | ||
dead.push_back(it); | ||
removed_size += entry.external_size; | ||
} | ||
entry.used_this_frame = false; | ||
} | ||
|
||
for (auto it : dead) { | ||
cache.erase(it); | ||
} | ||
return removed_size; | ||
} | ||
|
||
const size_t access_threshold_; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the threading restrictions that implementors must adhere to. I believe the shell is not thread-unsafe. Details below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what thread restrictions to document here. This is just called when there's a hint available for bytes freed - shouldn't it be up to the implementer what threads it uses when processing that information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant whoever implements the delegate protocol will receive events on the raster thread. This might be unexpected to the implementer.