Last Comment Bug 691775 - 3.3% performance regression in tscroll suite on XP after landing of bug 666446
: 3.3% performance regression in tscroll suite on XP after landing of bug 666446
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Windows XP
-- normal with 2 votes (vote)
: ---
Assigned To: Scott Johnson (:jwir3)
: Milan Sreckovic [:milan]
Depends on:
Blocks: 666446
  Show dependency treegraph
Reported: 2011-10-04 08:41 PDT by Scott Johnson (:jwir3)
Modified: 2012-01-05 13:03 PST (History)
16 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

b691775 - Patch (v1) (2.78 KB, patch)
2011-11-02 15:10 PDT, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review

Description User image Scott Johnson (:jwir3) 2011-10-04 08:41:48 PDT
Talos suites for winXP after bug 666446 landed is showing a regression in tscroll and tdhtml:
tdhtml: 723.68 ([{%22test%22:%2218%22,%22branch%22:%221%22,%22machine%22:%22507%22,%22testrun%22:%228555367%22}])
tscroll: 14618.1 ([{%22test%22:%2271%22,%22branch%22:%221%22,%22machine%22:%22507%22,%22testrun%22:%228555371%22}])

Previous check-in results were:
    tdhtml: 539.82 ([{%22test%22:18,%22branch%22:1,%22machine%22:667,%22testrun%22:8549730}])
    tscroll: 11570.2 ([{%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 User image Scott Johnson (:jwir3) 2011-11-01 14:46:18 PDT
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 User image Timothy Nikkel (:tnikkel) 2011-11-01 19:16:12 PDT
There was a tscroll regression email for XP that included these patches, although it is much smaller.
Comment 3 User image Scott Johnson (:jwir3) 2011-11-01 20:12:53 PDT
Ok, I'm reopening this bug to track this issue.
Comment 4 User image Scott Johnson (:jwir3) 2011-11-02 09:01:51 PDT
So, it looks like this is still being caused by nsImageLoader. I intend to have this isolated and a patch posted today.
Comment 5 User image Scott Johnson (:jwir3) 2011-11-02 14:46:46 PDT
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:

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 User image Scott Johnson (:jwir3) 2011-11-02 15:10:55 PDT
Created attachment 571472 [details] [diff] [review]
b691775 - Patch (v1)
Comment 7 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-02 15:20:35 PDT
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 User image Scott Johnson (:jwir3) 2011-11-10 07:21:56 PST
This should no longer be an issue with bug 666446. Please reopen if I am incorrect.
Comment 9 User image Alex Keybl [:akeybl] 2012-01-05 12:50:33 PST
Tracking bug 666446 instead, but this bug may still be reopened for FF10.

Note You need to log in before you can comment on or make changes to this bug.