DHTML performance regression with this particular testcase because of the fix for bug 261064

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

({perf, regression, testcase})

Trunk
mozilla1.8beta1
perf, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
This is a spin-off, from bug 277044.
The fix for bug 277044 probably caused a performance regression with this testcase.
See for the test results in the url testcase.
A 2004-11-14 7:17am Firefox build takes 8032ms for the relative/transparent
image case.
A 2004-11-15 7:48am Firefox build takes 24255ms for the relative/transparent
image case.

Also, I've used an old 2004-11-11 (debug) build.
Before I applied the fix for bug 261064, I get 8044ms for the position:relative
case/transparent image case.
After I applied the fix, I get 12568ms (and a high cpu use).
(Reporter)

Comment 1

13 years ago
Correction, I meant: "he fix for bug 261064 probably caused ..."
I see greatly increased CPU usage on this testcase on Linux too...  I'm not sure
why it's happening; we should be able to optimize this just as well as we used
to, since the abs pos div doesn't have auto offsets...
Assignee: nobody → roc
Flags: blocking1.8b?
OS: Windows 2000 → All
Hardware: PC → All
Created attachment 172189 [details] [diff] [review]
This is a start

So the testcase basically looks like this:

<body>
 <table>
   <div style="position: relative">
     <div style="position: absolute">This is animated</div>
   </div>
 </table>

 <div style="position: absolute">Auto offsets, little content</div>
 <div style="position: absolute">Non-auto offsets, lots of content</div>
</body>

So when we were reflowing the body, we saw that we had a reflow path that went
through a non-abs-pos element (the table), so we had to do a real reflow. Then
when we checked whether we have an abs pos kid that has auto offsets we found
that we do, so we reflowed all abs pos kids, which took time.

The patch does the following:

1)  Fix the FramesDependOnContainer function to not be so eager to return
"true"
    if just one offset is auto.
2)  Fix the XXX comment before that function so a single auto-offset frame
won't
    cause all abs pos kids to be reflown.  This was the first thing I tried,
    then I realized that there were frames that had both offsets auto involved.


This seems to help some.  It'd be even better if we could detect that our
reflow path just goes to an abs pos element so we really don't have to bother
with a reflow, but I'm not quite sure how to best do this yet.

The table also makes life annoying because of the MEW-computation (removing it
really decreases CPU usage after this patch is applied), but I'm also not sure
what I can do about that...
Attachment #172189 - Flags: superreview?(roc)
Attachment #172189 - Flags: review?(roc)
Er...  The "This was the first thing I tried" comment belongs with change #1,
not change #2.
Comment on attachment 172189 [details] [diff] [review]
This is a start

You could write AddFrameToChildBounds more simply as

if (!aChildBounds) return;
nsRect kidBounds = aKidFrame->GetRect();
nsRect* kidOverflow = aKidFrame->GetOverflowAreaProperty();
if (kidOverflow) {
  kidBounds = *kidOverflow + kidBounds.TopLeft();
}
aChildBounds->UnionRect(*aChildBounds, kidBounds);

OK, I see you're just moving/coalescing it. So that's an optional fix...

Other than that, it looks great.
Attachment #172189 - Flags: superreview?(roc)
Attachment #172189 - Flags: superreview+
Attachment #172189 - Flags: review?(roc)
Attachment #172189 - Flags: review+
Assignee: roc → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Checked in.

Marking fixed, but if there are followup bugs, please file them....
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

13 years ago
Well, these are my results (maybe useful):

Results build:       01-23   01-24
relative: transpare: 40238   40208
absolute: transpare: 36453   37213
fixed   : transpare: 36603   37544
static  : transpare: 8032    8032
relative: not_trans: 8082    8062
absolute: not_trans: 8032    8032
fixed   : not_trans: 8032    8032
static  : not_trans: 8022    8022
relative: div_block: 8032    8022
absolute: div_block: 8022    8032
fixed   : div_block: 8032    8022
static  : div_block: 8022    8032

So the relative case seems a bit faster now.
Odd.  This patch actually helped out CPU usage a lot on my machine...  Sounds
like most of the transparent slowness is coming from somewhere else?
(Reporter)

Comment 9

13 years ago
I guess so, probably bug 277762 is hitting me quite hard.

Updated

13 years ago
Flags: blocking1.8b?
You need to log in before you can comment on or make changes to this bug.