Optimize RebuildImageVisibilityDisplayList using nsTHashtable::SwapElements

RESOLVED FIXED in mozilla38

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

PresShell::RebuildImageVisibilityDisplayList() currently enumerates
mVisibleImages copying them over to a nsTArray and then remove
the entries one by one.  This can be made faster by swapping out
the elements of mVisibleImages with an empty temp hashtable
which can then be enumerated at the end.

http://hg.mozilla.org/mozilla-central/annotate/bb8d6034f5f2/layout/base/nsPresShell.cpp#l5820
A highly unscientific micro benchmark shows that this is ~25% faster than
the replaced code.  As a testcase I was just scrolling http://lazorz.com/
(Warning: might be NSFW) up and down for while.  Most of the calls have
zero images in mVisibleImages though, with occasional 20-50 or so.
I can measure again if you know a better stress test.

I was just measuring the code that manipulates the table/array before
and after the MarkImagesInListVisible/MarkImagesInSubtreeVisible calls,
which is just a fraction of the RebuildImageVisibility time of course.
Still, it's a small improvement, and the new code is slightly simpler IMO.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9714f259a95
Attachment #8547172 - Flags: review?(tnikkel)
Comment on attachment 8547172 [details] [diff] [review]
part 2 - Optimize RebuildImageVisibility[DisplayList]() using nsTHashtable::SwapElements

Thanks for doing this.

We already have a decrement iterator for the hashtable, it's called DecrementVisibleCount, so just use that one.
Attachment #8547172 - Flags: review?(tnikkel) → review+
Using the existing DecrementVisibleCount.
Attachment #8547172 - Attachment is obsolete: true
Attachment #8547535 - Flags: review+
Attachment #8547169 - Flags: review?(benjamin)
Attachment #8547169 - Flags: review?(benjamin) → review?(nfroyd)
Comment on attachment 8547169 [details] [diff] [review]
part 1 - Introduce nsTHashtable::SwapElements for a fast way to swap the elements of two hashtables

Review of attachment 8547169 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsTHashtable.h
@@ +293,5 @@
> +   * Swap the elements in this hashtable with the elements in aOther.
> +   */
> +  void SwapElements(nsTHashtable<EntryType>& aOther)
> +  {
> +    mozilla::Swap(this->mTable, aOther.mTable);

As a check that we're not doing something gruesome, can you add something like:

  MOZ_ASSERT_IF(this->mTable.ops && aOther.mTable.ops, this->mTable.ops == aOther.mTable.ops);

(probably with adding #include "mozilla/Assertions.h" to this file)?  It's not a perfect check, but it's better than nothing...
Attachment #8547169 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/62bd58692f27
https://hg.mozilla.org/mozilla-central/rev/984560bfd157
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.