Closed Bug 1620005 Opened 4 years ago Closed 4 years ago

Move display item caching logic to DisplayListBuilder

Categories

(Core :: Graphics: WebRender, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: mikokm, Assigned: mikokm)

References

Details

Attachments

(1 file)

This improves the separation of concerns of the implementation. The actual result of caching should be the same. The change was needed to allow caching decision to be moved inside nsDisplayItem::CreateWebRenderCommands() (or anywhere with DisplayItemBuilder in scope), instead of doing it by per-item basis beforehand.

Old design when caching an item:

  • WebRenderCommandBuilder::CreateWebRenderCommands() notified DisplayItemCache that nsDisplayItem::CreateWebRenderCommands() was about to be called.
  • DisplayItemCache assigned the cache slots to the item and notified DisplayItemBuilder if the item supported caching.
  • WebRenderCommandBuilder::CreateWebRenderCommands() notified DisplayItemCache that nsDisplayItem::CreateWebRenderCommands() was finished.
  • DisplayItemCache used DisplayItemBuilder to push additional items, if the item was cached.

New design:
DisplayItemBuilder now has methods:

void StartGroup(nsPaintedDisplayItem* aItem);
void CancelGroup();
void FinishGroup();
bool ReuseItem(nsPaintedDisplayItem* aItem);

Any WR display items created between calls to StartGroup() and FinishGroup() will be reused by a call to ReuseItem(), which will push a DisplayItem::ReuseItem(key) to WR display list. This call is (still) inside WebRenderCommandBuilder::CreateWebRenderCommands(). A call to CancelGroup() will discard display items pushed after the call to StartGroup().

For example, inside nsDisplayBackgroundColor::CreateWebRenderCommands():

aBuilder.StartGroup(this);
aBuilder.PushRect(r, r, !BackfaceIsHidden(),
                  wr::ToColorF(ToDeviceColor(mColor)));
aBuilder.FinishGroup();

DisplayItemCache is now only responsible for assigning the cache slots and ensuring that the cache state is valid. This means handling some awkward edge-cases like changing pipeline/clip/spatial ids, which require cache invalidation. Additionally it is an optimization to avoid bloating gecko display items with additional state data.

Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/91760460f914
Refactor WebRender display item caching r=jrmuizel
Flags: needinfo?(mikokm)
Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f792d895cdf8
Refactor WebRender display item caching r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76

(In reply to Markus Stange [:mstange] from comment #6)

Could this have caused a performance improvement? I see a big step down on the displaylist_mutate test when this landed: https://treeherder.mozilla.org/perf.html?selectMulti=displaylist_mutate+opt+e10s+stylo#/graphs?highlightAlerts=1&selected=1937901,1073835236&series=mozilla-central,1937901,1,1&series=mozilla-central,1934058,1,1&timerange=7776000&zoom=1583907509944,1584031730802,1273.26814920112,1625.1326319897125

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c982daa151954c59f20a9b9ac805c1768a350c2&tochange=4fd5c458be4c3bc2d1f22bd575667104a5d173fe

I think that bump was caused by bug 1619461.

The improvements from this feature were similar though (for WR display list bound benchmarks), but reported in bug 1616412, where the pref was flipped on.

Ah, of course.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: