Closed Bug 1120198 Opened 7 years ago Closed 7 years ago

Optimize RebuildImageVisibilityDisplayList using nsTHashtable::SwapElements

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(2 files, 1 obsolete file)

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+
You need to log in before you can comment on or make changes to this bug.