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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(1 obsolete attachment)

Updated

8 years ago
Summary: Performance regression in talos suites → Performance regression in talos suites on XP after landing of bug 666446

Updated

8 years ago
(Assignee)

Comment 1

8 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: 8 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

8 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

8 years ago
Assignee: nobody → sjohnson
(Assignee)

Comment 4

8 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

8 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

8 years ago
Posted patch b691775 - Patch (v1) (obsolete) — Splinter Review
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

8 years ago
This should no longer be an issue with bug 666446. Please reopen if I am incorrect.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Tracking bug 666446 instead, but this bug may still be reopened for FF10.
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.