Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Blocks: 666446
Summary: Performance regression in talos suites → Performance regression in talos suites on XP after landing of bug 666446
tracking-firefox10: --- → ?
(Assignee)

Comment 1

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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#
(Assignee)

Comment 3

6 years ago
Ok, I'm reopening this bug to track this issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Performance regression in talos suites on XP after landing of bug 666446 → 3.3% performance regression in tscroll suite on XP after landing of bug 666446
(Assignee)

Updated

6 years ago
Assignee: nobody → sjohnson
(Assignee)

Comment 4

6 years ago
So, it looks like this is still being caused by nsImageLoader. I intend to have this isolated and a patch posted today.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
Created attachment 571472 [details] [diff] [review]
b691775 - Patch (v1)
Attachment #571472 - Flags: review?(roc)
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?
(Assignee)

Comment 8

6 years ago
This should no longer be an issue with bug 666446. Please reopen if I am incorrect.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 9

6 years ago
Tracking bug 666446 instead, but this bug may still be reopened for FF10.
tracking-firefox10: ? → -

Updated

6 years ago
Attachment #571472 - Attachment is obsolete: true
Attachment #571472 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.