Details

Reviewers

Commits

rMOZILLACENTRAL39db0be47ff6: Bug 1605283 - Improve support for invalidation debugging and testing r=gw
rMOZILLACENTRAL77d7f22c81c4: Bug 1605283 - Improve support for invalidation debugging and testing r=gw
rMOZILLACENTRAL7ed018d13880: Bug 1605283 - Improve support for invalidation debugging and testing r=gw
rMOZILLACENTRAL2e04c320ad23: Bug 1605283 - Improve support for invalidation debugging and testing r=gw

Bugzilla Bug ID

1605283

Summary

Second part: trace the updates that are sent to the DataStore, and save
at least the Insert/Remove and ItemUID as part of the wr-capture.
(We could expand this with more info, eg. the actual Keys, later).

TileView then reads them back and generates a color coded report to
overlay with the page view. This helps to see the types and amounts of
interned primitives that lead to cache invalidations.

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.Jan 22 2020, 1:09 AM

Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 1:09 AM

Harbormaster completed remote builds in B155417: Diff 220625.Jan 22 2020, 1:09 AM

phab-bot requested review of this revision.Jan 22 2020, 1:09 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.

Hello again, I've added logging of the interning activity on the DataStore. Instead of implementing on_interned for various keys, I decided to directly serialize out the UpdateList's Vec<Update>. Each frame has each interner save out any activity, and the viewer then loads it back in, showing what each frame did. The image below shows the result.

There is one frame of delay: I report interning of a bunch of new primitives on frame N, and only frame N+1 shows the tiles invalidating. This might be normal due to pipelining, or maybe I have a bug :) In any case, you can see the number and type of primitives that gets inserted just before/during cache invalidations.

We could perhaps also save out the Keys, but then I suspect we might have to spam a lot of Default and Clone attributes. I went with the approach of directly saving Vec<Update> and no more, precisely because I wasn't too sure how deep that rabbit hole would be.

Anyway, I'm just experimenting with adding more useful info, please let me know your thoughts! Thanks!

Edit: and a video link in case I'm not making much sense :)
https://drive.google.com/open?id=1QmdD0nvD6nfKVn-KeDA7ZQbjFysLoIkA

gfx/wr/tileview/src/main.rs
395	Generates an HTML file, one per frame, that shows each itemUID colorcoded -- green if added this frame, red if removed.
484	Multiple pieces of already-serialized .ron go into a single file, so I'm delineating them with magic strings, and using split to break them apart again, then de-serialize each individually. The reason is that they're sourced from different places, and it would get tricky to artificially try to force the types into a single structure just to have a one-line serialize. So... this works, I guess.
485	Have removed this.
gfx/wr/webrender/src/intern.rs
57	Even with a `serde(skip)` on `data`, I was getting complaints from rustc when I tried to make `UpdateList` serialize/clone/default, so that's why I'm manipulating `updates` directly.

gw added inline comments.Jan 22 2020, 4:50 AM

gfx/wr/tileview/src/main.rs
484	Instead of serializing them into a single file with magic splits, perhaps we could serialize them into multiple (numbered?) files - that seems like it would be a bit tidier?
gfx/wr/webrender/src/intern.rs
57	Could you post the compile error, just out of interest?
gfx/wr/webrender/src/picture.rs
1559	Related to the above comment, perhaps this returns a `Vec<String>` which then gets written to individual files?
1635–1637	nit: `_i` can just be `_`

• Bert marked an inline comment as done.Jan 22 2020, 7:18 PM

• Bert added inline comments.

gfx/wr/tileview/src/main.rs
484	Hmmm.... Sure, but, keep in mind we already have a file per frame, so 1000 frames of history * ( 13 interners + 1 tilecache) == 14000 files. Not sure if that's what you meant?
gfx/wr/webrender/src/intern.rs
57	Certainly; the change is #[cfg_attr(feature = "capture", derive(Serialize))] #[cfg_attr(feature = "replay", derive(Deserialize))] pub struct UpdateList<S> { /// The additions and removals to apply. pub updates: Vec<Update>, /// Actual new data to insert. #[cfg_attr(any(feature = "capture", feature = "replay"), serde(skip))] data: Vec<S>, } plus a impl<S> UpdateList<S> { pub fn new() -> Self { Self { updates: Vec::new(), data: Vec::new() } } } so I can create an empty instance of `TileCacheLoggerUpdateLists` with a `pub $name: (String, UpdateList<$ty>)` per interner; the result is: error[E0277]: the trait bound `api::PrimitiveKeyKind: std::default::Default` is not satisfied --> webrender\src\picture.rs:1555:42 \| 1555 \| self.$name.1 = match ron::de::from_str(&list.unwrap()) { \| ^^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `api::PrimitiveKeyKind` ... 1569 \| enumerate_interners!(declare_tile_cache_logger_updatelists); \| ------------------------------------------------------------ in this macro invocation \| ::: C:\Users\Kris Taeleman\.cargo\registry\src\github.com-1ecc6299db9ec823\ron-0.1.7\src\de\mod.rs:62:14 \| 62 \| where T: de::Deserialize<'a> \| ------------------- required by this bound in `ron::de::from_str` \| = note: required because of the requirements on the impl of `serde::Deserialize<'_>` for `intern::UpdateList<api::PrimitiveKeyKind>` = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) ... 13 times for each $ty. So that's a bit odd :)
gfx/wr/webrender/src/picture.rs
1635–1637	👍

gw accepted this revision.Jan 22 2020, 8:13 PM

gw added inline comments.

gfx/wr/tileview/src/main.rs
484	I hadn't realized quite how many frames history we keep :) Another idea then: We add: [derive(capture stuff)] struct InternerData { ron_string: Vec<String>, } And then instead of using a magic marker, we let RON serialize a single struct that contains an array of strings, each of which is a RON struct serialized to a string. Would that work, and be a bit tidier than manually managing the `@@@` markers ourselves?
gfx/wr/webrender/src/intern.rs
57	I think if you add a `Default` trait bound to the struct, e.g.: pub struct UpdateList<S: Default> { ... That might allow you to work with it - but it was more out of curiosity than anything, nothing we need to do here for this patch.

This revision is now accepted and ready to land.Jan 22 2020, 8:13 PM

• Bert updated this revision to Diff 221283.Jan 23 2020, 12:11 AM

• Bert marked 6 inline comments as done.

Herald added a subscriber: Itiel. · View Herald TranscriptJan 23 2020, 12:11 AM

Harbormaster completed remote builds in B155840: Diff 221283.Jan 23 2020, 12:11 AM

• Bert added inline comments.Jan 23 2020, 12:12 AM

gfx/wr/tileview/src/main.rs

484

Great suggestion, thanks!

gfx/wr/webrender/src/intern.rs

That seems to make it more explicit about the missing Defaults:

webrender\src\intern.rs|63 col 9 error 277| the trait bound `S: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `clip::ClipItemKey: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimitiveKey: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::borders::NormalBorderPrim>: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::borders::ImageBorder>: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::image::Image>: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::image::YuvImage>: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::line_dec::LineDecoration>: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::gradient::LinearGradientKey: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::gradient::RadialGradientKey: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::picture::Picture>: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::text_run::TextRunKey: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `filterdata::SFilterDataKey: std::default::Default` is not satisfied
webrender\src\scene_builder_thread.rs|200 col 17 error 277| the trait bound `prim_store::PrimKey<prim_store::backdrop::Backdrop>: std::default::Default` is not satisfied

I guess I _could_ go and add all of those (and their field subtypes and so on), I might have no choice if we start (de)serializing the keys as well.

(The alternative option for getting more info in the log than just a number would be to implement on_interned and have it write a String to the log as well. A bit of a plumbing hassle to pass the logger around, but otherwise less invasive than pub/Default/Serialize/Deserialize a ton of structures?)

Closed by commit rMOZILLACENTRAL2e04c320ad23: Bug 1605283 - Improve support for invalidation debugging and testing r=gw (authored by • Bert). · Explain WhyJan 23 2020, 2:17 AM

This revision was automatically updated to reflect the committed changes.

• Bert added a commit: rMOZILLACENTRAL2e04c320ad23: Bug 1605283 - Improve support for invalidation debugging and testing r=gw.

aciure reopened this revision.Jan 23 2020, 2:34 AM

This revision is now accepted and ready to land.Jan 23 2020, 2:34 AM

aciure added a reverting change: rMOZILLACENTRALaa4d123de678: Backed out changeset 2e04c320ad23 (bug 1605283) for causing build bustages….Jan 23 2020, 2:34 AM

• Bert updated this revision to Diff 221387.Jan 23 2020, 6:22 AM

Harbormaster completed remote builds in B155898: Diff 221387.Jan 23 2020, 6:22 AM

Closed by commit rMOZILLACENTRAL7ed018d13880: Bug 1605283 - Improve support for invalidation debugging and testing r=gw (authored by • Bert). · Explain WhyJan 23 2020, 6:26 AM

This revision was automatically updated to reflect the committed changes.

• Bert added a commit: rMOZILLACENTRAL7ed018d13880: Bug 1605283 - Improve support for invalidation debugging and testing r=gw.

• dvarga reopened this revision.Jan 23 2020, 6:51 AM