[FIX]Consider caching the IsEmpty state of textframes

RESOLVED FIXED in mozilla1.8alpha3

Status

()

Core
Layout: Misc Code
P1
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({perf})

Trunk
mozilla1.8alpha3
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I was profiling changing the product on the bugzilla query page (which kicks off
a bunch of reflows) and found that a lot of time (20% or so) was being spent
under nsTextFrame::IsEmpty (getting style data, QIing the content node, calling
IsOnlyWhitespace, etc).

There were a lot of repeat calls, so it may make sense to cache the IsEmpty
state on the frame itself.
Created attachment 155238 [details] [diff] [review]
Like so, perhaps
Comment on attachment 155238 [details] [diff] [review]
Like so, perhaps

I have to admit that I don't know how much overall impact this will have on
things.  For the testcase I was looking at, the time in nsTextFrame::IsEmpty
dropped by a factor of 10 or so.  The cached state was hit almost every single
time (only 3% of the time under IsEmpty() was spent actually getting style data
or calling IsOnlyWhitespace or any of that stuff).
Attachment #155238 - Flags: superreview?(roc)
Attachment #155238 - Flags: review?(roc)
Blocks: 183918
Keywords: perf
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha3
Comment on attachment 155238 [details] [diff] [review]
Like so, perhaps

+  NS_ASSERTION(mState & TEXT_IS_EMPTY == 0 ||
+		mState & TEXT_ISNOT_EMPTY == 0, "Invalid state");

I think this is incorrect since == has higher precendence than &. So you're
getting (mState & (TEXT_IS_EMPTY == 0)) || (mState & (TEXT_ISNOT_EMPTY == 0))
which should always be firing...

Well, even if I'm wrong, I prefer the clearer !(mState & TEXT_IS_EMPTY) ||
!(mState & TEXT_ISNOT_EMPTY)
Attachment #155238 - Flags: superreview?(roc)
Attachment #155238 - Flags: superreview+
Attachment #155238 - Flags: review?(roc)
Attachment #155238 - Flags: review+
Made that change and checked in.  No Tp impact.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Reopening.  It looks like Tp on btek did go up a bit after all.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 155729 [details] [diff] [review]
Trying to not regress Tp (apply on top of other patch)
Attachment #155238 - Attachment is obsolete: true
Attachment #155729 - Flags: superreview+
Attachment #155729 - Flags: review+
Yeah, that helped Tp.  Fixed.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.