Change LayerManagerData::mDisplayItems to a vector instead of hashtable #1860

Open
opened 4 months ago by win7-7 · 7 comments

https://bugzilla.mozilla.org/show_bug.cgi?id=1468426

This should improve cache locality and help speed up layer building. Uses vector instead of hashtable.

// Restore DisplayItemData is used in layer flattening so it need also be changed to vector instead hashtable.

// Restore PaintedLayerItemEntries has its own hashtable defined in FrameLayerBuilder.h nsTHashtable<PaintedLayerItemsEntry> mPaintedLayerItems; so PaintedLayerItemEntries does not need be changed to vector to get this patch backported.

Maze benchmark (when tested with -GL/Whole Program Optimization):

table test is 10-11 titles faster.
notable test is 5-10 titles faster.
Float is about 9-10 titles faster.

PR would do change for vector.

https://bugzilla.mozilla.org/show_bug.cgi?id=1468426 This should improve cache locality and help speed up layer building. Uses vector instead of hashtable. // Restore DisplayItemData is used in layer flattening so it need also be changed to vector instead hashtable. // Restore PaintedLayerItemEntries has its own hashtable defined in FrameLayerBuilder.h `nsTHashtable<PaintedLayerItemsEntry> mPaintedLayerItems; ` so PaintedLayerItemEntries does not need be changed to vector to get this patch backported. Maze benchmark (when tested with -GL/Whole Program Optimization): table test is 10-11 titles faster. notable test is 5-10 titles faster. Float is about 9-10 titles faster. PR would do change for vector.
Moonchild added the
Performance
label 4 months ago
Owner

Do you have a link to this "maze benchmark"?

Do you have a link to this "maze benchmark"?
Poster
http://sinz.org/Maze/
Owner

Did you also take https://bugzilla.mozilla.org/show_bug.cgi?id=1469472 into account which is a crash regression from the listed bug?

Did you also take https://bugzilla.mozilla.org/show_bug.cgi?id=1469472 into account which is a crash regression from the listed bug?
Poster

Yes I did look into it but there is no CanMerge or HasSameTypeAndClip in UXP and testcase of 1469472 does not crash in UXP with vector used.

Yes I did look into it but there is no CanMerge or HasSameTypeAndClip in UXP and testcase of 1469472 does not crash in UXP with vector used.
Owner

Thanks - However this is security sensitive and the main issue seems to be that frames get added to the manager twice and removed twice, which a hashtable allows but a vector does not. This is unrelated to the helper functions you're talking about that we don't have (and Mozilla actually removed in this case).

I'll add some DiD code myself after merging.

Thanks - However this is security sensitive and the main issue seems to be that frames get added to the manager twice and removed twice, which a hashtable allows but a vector does not. This is unrelated to the helper functions you're talking about that we don't have (and Mozilla actually removed in this case). I'll add some DiD code myself after merging.
Moonchild added the
Fixed
Verified
labels 4 months ago
Owner

Verified performance comparison with the maze test, confirming improvements.

Verified performance comparison with the maze test, confirming improvements.
Moonchild closed this issue 4 months ago
Moonchild reopened this issue 4 months ago
Moonchild added
Backed Out
and removed
Fixed
Verified
labels 4 months ago
Owner

I'm backing this out. It does seem to be hitting the sec issue even with my DiD code as @FranklinDM noticed, causing potentially exploitable crashes. He came up with a solution to do it in a safe way (checking for existence of elements in the display list of vectors before adding/removing) but the result of that is that we'd lose all the gained performance by moving to vector and more (a comparison on my build system showed a loss of almost 20% performance on the fixed seed benchmark compared to hashtable, instead of gaining performance).

I'm leaving this open for a potentially better performing solution, but I'm not taking this for 31.0 as-is.

I'm backing this out. It does seem to be hitting the sec issue even with my DiD code as @FranklinDM noticed, causing potentially exploitable crashes. He came up with a solution to do it in a safe way (checking for existence of elements in the display list of vectors before adding/removing) but the result of that is that we'd lose all the gained performance by moving to vector and more (a comparison on my build system showed a loss of almost 20% performance on the fixed seed benchmark compared to hashtable, instead of gaining performance). I'm leaving this open for a potentially better performing solution, but I'm not taking this for 31.0 as-is.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: MoonchildProductions/UXP#1860
Loading…
There is no content yet.