691775 - 3.3% performance regression in tscroll suite on XP after landing of bug 666446

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Scott Johnson (:jwir3)]

Talos suites for winXP after bug 666446 landed is showing a regression in tscroll and tdhtml:
   
tdhtml: 723.68 (http://graphs.mozilla.org/graph.html#type=series&tests=[{%22test%22:%2218%22,%22branch%22:%221%22,%22machine%22:%22507%22,%22testrun%22:%228555367%22}])
tscroll: 14618.1 (http://graphs.mozilla.org/graph.html#type=series&tests=[{%22test%22:%2271%22,%22branch%22:%221%22,%22machine%22:%22507%22,%22testrun%22:%228555371%22}])

Previous check-in results were:
    tdhtml: 539.82 (http://graphs.mozilla.org/graph.html#type=series&tests=[{%22test%22:18,%22branch%22:1,%22machine%22:667,%22testrun%22:8549730}])
    tscroll: 11570.2 (http://graphs.mozilla.org/graph.html#type=series&tests=[{%22test%22:71,%22branch%22:1,%22machine%22:667,%22testrun%22:8549734}])

This looks like a 26% increase in time required to execute tscroll, and a 34% increase in time required for tdhtml, but only on WinXP.

Comment 1

[Scott Johnson (:jwir3)]

This was fixed with the latest patch to bug 666446. bug 666446 was backed out, and when recommitted, it will longer suffer from this issue.

Comment 2

[Timothy Nikkel (:tnikkel)]

There was a tscroll regression email for XP that included these patches, although it is much smaller.

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/965e116ddcaeea1a#

Comment 3

[Scott Johnson (:jwir3)]

Ok, I'm reopening this bug to track this issue.

Comment 4

[Scott Johnson (:jwir3)]

So, it looks like this is still being caused by nsImageLoader. I intend to have this isolated and a patch posted today.

Comment 5

[Scott Johnson (:jwir3)]

Ok.. I think I found the problem. I'll push a patch in a second for review, but the following Talos test results on try seem to show the problem:
https://tbpl.mozilla.org/?tree=Try&rev=02b9e850bc28

Basically, what's happening is that I tried to use the logic in nsLayoutUtils::DeregisterIfNotAnimated() in order to be consistent with the other invocations during Load, but having to register and then immediately deregister again is inefficient, so I did the animation check up front and only register if the new image is animated.

Comment 6

[Scott Johnson (:jwir3)]

Attachment: b691775 - Patch (v1)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 571472 [details] [diff] [review]
b691775 - Patch (v1)

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

It seems weird that this would cause a noticeable performance effect.

::: layout/base/nsImageLoader.cpp
@@ +152,5 @@
> +      bool isAnimated;
> +      nsresult rv = imageContainer->GetAnimated(&isAnimated);
> +      if (NS_SUCCEEDED(rv) && isAnimated) {
> +        nsLayoutUtils::RegisterImageRequest(presContext, mRequest,
> +                                            &mRequestRegistered);

Doesn't this mean that a partially loaded image is never registered?

Comment 8

[Scott Johnson (:jwir3)]

This should no longer be an issue with bug 666446. Please reopen if I am incorrect.

Comment 9

[Alex Keybl [:akeybl]]

Tracking bug 666446 instead, but this bug may still be reopened for FF10.