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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

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

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24e717192987

Comment 3

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c087893705b

Comment 4

[Seth Fowler [:seth] [:s2h]]

Ack, comment 3 is actually a try job for bug 1151359, posted in this bug by mistake.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8633698 [details] [diff] [review]
Only mark surfaces as used in the SurfaceCache if a caller requested exactly that surface

r=me

Comment 6

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

Comment 7

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d601c12a333d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d601c12a333d