Bug 1605283 - Improve support for invalidation debugging and testing
ClosedPublic
Actions

Authored by • Bert on Feb 5 2020, 7:52 AM.

Details

Reviewers

Commits

rMOZILLACENTRAL813768d074e3: Bug 1605283 - Improve support for invalidation debugging and testing r=gw

Bugzilla Bug ID

1605283

Summary

Fourth iteration: improve the detail in reported tile invalidations.

The invalidation enum stores the old and new values for lightweight
types. For a change in PrimCount, the old and new list of ItemUids is
stored (if logging is enabled); the tool can then diff the two to see
what was added and removed. To convert that back into friendly strings,
the interning activity is used to build up a map of ItemUid to a string.

A similar special-casing of Content Descriptor will print the item
that's invalidating the tile, plus the origin and/or rectangle.

Also adding zoom and pan command line options both to fix high-DPI
issues and also to allow zooming out far enough to see out-of-viewport
cache lifetime and prefetching due to scrolling.

Also fix a bug where interning updates get lost if more than one update
happens without building a frame: switch to a vector of serialized
updatelists (one per type) instead of allowing just one string (per
type).

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

• Bert created this revision.Feb 5 2020, 7:52 AM

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2020, 7:52 AM

Harbormaster completed remote builds in B158238: Diff 224670.Feb 5 2020, 7:53 AM

phab-bot requested review of this revision.Feb 5 2020, 7:53 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: Restricted Project.

Herald added a subscriber: Itiel. · View Herald TranscriptFeb 5 2020, 7:53 AM

Detailed information about which primitives were added and removed when the invalidation reason is PrimCount:

Detailed information about what changed when the reason is "Content: Descriptor".

Basically all invalidation reasons now come with some descriptive text, even if it's just a Debug print.

Also added pan and zoom (non-interactive: via tool cmdline arguments), I had some issues playing high-DPI captures back on a normal monitor -- but it also helps to step back and see cache activity outside the viewport a little better:

• Bert added inline comments.Feb 5 2020, 8:02 AM

gfx/wr/webrender/src/picture.rs
600	Instead of just a bool, is_same will return one of these.
1331	My assumption is that these are small enough and rare enough to not become a performance issue.
1714	My assumption is that ItemUids are globally unique across all types, so this map is a simple lookup from u32 to a debug-printed version of an interned key, one map for all key types.

gw requested changes to this revision.Feb 6 2020, 12:21 AM

gw added inline comments.

gfx/wr/webrender/src/intern.rs
71	There's a `get_uid` accessor intended for debug use, so we probably don't need to make this public?
gfx/wr/webrender/src/picture.rs
622	I think increasing the size of this enum is likely to cause a perf regression in this code. Perhaps we can provide the data a different way, only when tracing is enabled?
1331	I'm not sure this assumption holds :) I wonder if we could provide some kind of `Option<&mut CompareResult>` field that is only provided when tracing is enabled that gets written to, so that in the common path we're not returning much larger structs than we currently are?

This revision now requires changes to proceed.Feb 6 2020, 12:21 AM

• Bert updated this revision to Diff 225552.Feb 6 2020, 8:49 PM

Harbormaster completed remote builds in B158873: Diff 225552.Feb 6 2020, 8:49 PM

As always, thanks for taking the time to go through these!

gfx/wr/webrender/src/intern.rs
71	Oops, good catch, thanks!
gfx/wr/webrender/src/picture.rs
1331	Yeah that sounds like the safer thing to do. I've gone ahead and moved all new extra info into a separate Option argument.
5822	(If I use `{ }` here to have a single `cfg` for both, it introduces a scope and the variables are invisible :\| )
5831	I'm not sure when capture/replay are enabled; but if the answer is "On Nightly and Beta only", then with some luck the compiler should optimize away all the stuff I added for the regular build, when it detects the Option is always None?

gw accepted this revision.Feb 6 2020, 11:19 PM

gw added inline comments.

gfx/wr/webrender/src/picture.rs
1305	If this stuff ever shows up in profiles, an alternative might be to have two different `is_same` impls, so that the fast path one doesn't have any extra branches. Not something we need to worry about for now though!
5831	Capture is probably (?) enabled all the time I think - since I think you can do a WR capture even in release builds? (maybe only on nightly though?). I think we'd want to only do this if the trace debug option is enabled too - but worth verifying, I might be wrong here.

This revision is now accepted and ready to land.Feb 6 2020, 11:19 PM

• Bert updated this revision to Diff 225635.Feb 6 2020, 11:20 PM

• Bert edited the summary of this revision. (Show Details)

• Bert marked 2 inline comments as done.

Harbormaster completed remote builds in B158922: Diff 225635.Feb 6 2020, 11:20 PM

Sorry, I just dragged in one more minor fix -- switched over to a vector of updatelists since sometimes these come in without a matching frame build, so they got stomped.
Thanks!

gfx/wr/webrender/src/picture.rs
1305	Hmm, yeah, it's small enough that a bit of copy paste wouldn't hurt too much.
5831	I just gave it a quick try on a regular build, and ctrl-shift-3 doesn't seem to do anything, even though webrender is enabled (I can see the picture cache debug squares). So that's good news then, need to worry about the performance slightly less :)