Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upStore a queue of rasterized blobs instead for non-tiled images (take two) #3111
Conversation
|
FWIW, I was thinking about this approach some more and think it would probably be better to just keep a copy of the whole rasterized blob around in memory instead of having the accumulating queue of updates |
|
This is blocking a bunch of other things, so if we want to pursue another approach, let's first land it and then implement something else so that the other work can land. I'm rather neutral about whether we should retain the whole blob in memory and paint over it or do this queued thing. In principle the former feels cleaner. It might be harder to implement because it isn't clear which thread can own the retained rasterized blobs. Also in theory it involves copying more memory but I'm not sure whether that really matters. |
|
I agree that we should land this first. |
|
complete! all files reviewed, all discussions resolved
|
@nical looks reasonable! Are you confident to land this without a try push? |
|
I have both the confidence and the try push (although the latter has commits from another PR in addition to this one). |
|
well, ship it then! |
|
|
|
What's up @bors-servo ? |
|
Mac CI is busted. Some sort of expired cert problem when the worker tries to upload the log to taskcluster. |
|
@bors-servo retry |
Store a queue of rasterized blobs instead for non-tiled images (take two) The 3 commits from #3079 which were reverted in #3108 plus a commit that fixes the crash that motivated the backout. Bugzilla entry: https://bugzilla.mozilla.org/show_bug.cgi?id=1493177 In the texture cache we have an automatic and a manual eviction policy. The manual eviction policy ensures blob images don't get automatically evicted from the cache which could otherwise race with the asynchronous rasterization. The Texture cache was missing a tiny piece of code to ensure the eviction policy was respected in for shared cache entries (it does work for normal cache entries). I bet border images make uses of these shared entries. While scrolling the image eventually gets discarded and next time we see it we request the image. The problem is that when we decide whether a blob image is missing, we only look at whether there is an entry in the rasterized_blob_image map, but we don't check whether that entry contains any actual data to upload in its queue. Before the regressing commit, the bug would occur without crashing, but we wouldn't necessary upload all of the image (only the last available dirty region). With that commit we end up requesting something we think we have but don't have and later panic with an empty handed upload request. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3111) <!-- Reviewable:end -->
|
|
nical commentedSep 24, 2018
•
edited by larsbergstrom
The 3 commits from #3079 which were reverted in #3108 plus a commit that fixes the crash that motivated the backout.
Bugzilla entry: https://bugzilla.mozilla.org/show_bug.cgi?id=1493177
In the texture cache we have an automatic and a manual eviction policy. The manual eviction policy ensures blob images don't get automatically evicted from the cache which could otherwise race with the asynchronous rasterization. The Texture cache was missing a tiny piece of code to ensure the eviction policy was respected in for shared cache entries (it does work for normal cache entries). I bet border images make uses of these shared entries. While scrolling the image eventually gets discarded and next time we see it we request the image. The problem is that when we decide whether a blob image is missing, we only look at whether there is an entry in the rasterized_blob_image map, but we don't check whether that entry contains any actual data to upload in its queue.
Before the regressing commit, the bug would occur without crashing, but we wouldn't necessary upload all of the image (only the last available dirty region). With that commit we end up requesting something we think we have but don't have and later panic with an empty handed upload request.
This change is