Closed Bug 147938 Opened 22 years ago Closed 22 years ago

Don't Invalidate nsImageFrame area if it's hidden

Categories

(Core :: Layout: Images, Video, and HTML Frames, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: paper, Assigned: paper)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

When nsImageFrame::FrameChanged is called, and the image frame is visibility:
hidden, we don't need to invalidate.  This saves a lot of calls and reduces CPU
usage.

Test Case:
http://www.animecity.nu/mozilla/IMGTest/hiddenStorms.html

There are 7 hidden animated gifs in the test case.  Without this patch, CPU is
about 25% on my system (PIII-933 on Win2k).  With the patch, CPU is 0%
Attached patch Patch to check if invisible (obsolete) — Splinter Review
I wouldn't mind someone confirming CPU usage on
http://www.animecity.nu/mozilla/IMGTest/hiddenStorms.html
Include CPU Speed, CPU Usage, OS, Build.
Depends on: 86319
No longer depends on: 86319
Blocks: 94828
I do see about 25% CPU usage on win XP trunk build 2002052908
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch, perf, review
os/2 mozilla rc2 plus privates, 700mhz, 
w/o fix: idle: 56.62%  busy: 41.99%  intr:  1.39%
w/ fix: idle: 78.91%  busy: 20.51%  intr:  0.58%
Blocks: 119597
Keywords: mozilla1.0.1
Comment on attachment 85476 [details] [diff] [review]
Patch to check if invisible

It might be nice to solve this problem more generally.	However, this seems OK
if you think it will help more than it hurts.  (How often are images hidden? 
How long does it take to check the style data?	It probably does...)

>+  const nsStyleVisibility* vis = 
>+        (const nsStyleVisibility*)((nsIStyleContext*)mStyleContext)->GetStyleData(eStyleStruct_Visibility);

Could you use

const nsStyleVisibility* vis;
::GetStyleData(this, &vis);

instead?  You certainly don't need that second cast.
David Baron:  Done.

The whole style casting thing is also done in about 6 other places in
nsImageFrame, and that's why I had it like that originally.  Is there enough
performace inprovements using your method to create another bug to convert
those 6 over?
Attachment #85476 - Attachment is obsolete: true
Comment on attachment 88265 [details] [diff] [review]
Patch to check if invisible

r=dbaron.  It's not a performance issue -- it's a typesafety one.
Attachment #88265 - Flags: review+
Comment on attachment 88265 [details] [diff] [review]
Patch to check if invisible

sr=waterson, nice!
Attachment #88265 - Flags: superreview+
fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.