1120198 - Optimize RebuildImageVisibilityDisplayList using nsTHashtable::SwapElements

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - Introduce nsTHashtable::SwapElements for a fast way to swap the elements of two hashtables

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - Optimize RebuildImageVisibility[DisplayList]() using nsTHashtable::SwapElements

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

Comment 3

[Timothy Nikkel (: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.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - Optimize RebuildImageVisibility[DisplayList]() using nsTHashtable::SwapElements

Using the existing DecrementVisibleCount.

Comment 5

[Nathan Froyd [:froydnj]]

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...

Comment 6

[Mats Palmgren (inactive)]

With the suggested assertion + #include:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62bd58692f27
https://hg.mozilla.org/integration/mozilla-inbound/rev/984560bfd157

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62bd58692f27
https://hg.mozilla.org/mozilla-central/rev/984560bfd157