Closed Bug 1183852 Opened 5 years ago Closed 5 years ago

Only mark surfaces as used in the SurfaceCache if a caller requested exactly that surface

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file)

The SurfaceCache is intended to only consider a surface used if some caller is actually, well, using it. However, that's not what's happening in practice, because:

(1) SurfaceCacheImpl::Insert calls SurfaceCacheImpl::Lookup, which marks the looked-up surface used. This is incorrect because Insert() is primarily called by decoders, and if we happen to create multiple decoders for the same SurfaceKey, the decoders which lose the race to create the surface should not mark that surface as used.

(2) SurfaceCacheImpl::LookupBestMatch marks surfaces as used whether it's returning an exact match or not. This is a bad idea, because it can greatly increase the lifetime of a surface that we no longer need, just because we happened to call Draw() one time before the new surface finished decoding.

Let's fix these issues.
Blocks: 1151359, 1183390
Here's the patch. SurfaceCacheImpl::Lookup() gets a new boolean aMarkUsed
parameter that determines whether it should mark the resulting surface used, and
we pass false for this parameter in SurfaceCacheImpl::Insert().
SurfaceCacheImpl::LookupBestMatch() only marks the resulting surface used if
it's an exact match.

To avoid some ugly nested |if| constructs, I've gone ahead and moved the code
that marks a surface used into its own private method,
SurfaceCacheImpl::MarkUsed(). This seemed too trivial for its own patch.
Attachment #8633698 - Flags: review?(dholbert)
Ack, comment 3 is actually a try job for bug 1151359, posted in this bug by mistake.
Comment on attachment 8633698 [details] [diff] [review]
Only mark surfaces as used in the SurfaceCache if a caller requested exactly that surface

r=me
Attachment #8633698 - Flags: review?(dholbert) → review+
Thanks for the review!
Blocks: 1166136
https://hg.mozilla.org/mozilla-central/rev/d601c12a333d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.