Open Bug 466300 Opened 16 years ago Updated 2 years ago

Reduce QI and AddRef/Release in nsFind.cpp

Categories

(Core :: Find Backend, defect)

defect

Tracking

()

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Whiteboard: p=0)

Attachments

(1 file, 1 obsolete file)

Based on a Shark profile AddRef/Release causes significant overhead
when using type-ahead-find, especially if the page doesn't contain the searched
word.
Patch coming.
Attached patch this should help a bit (obsolete) — Splinter Review
Attachment #349581 - Flags: superreview?(peterv)
Attachment #349581 - Flags: review?(peterv)
Comment on attachment 349581 [details] [diff] [review]
this should help a bit

It should be fairly easy to make SkipNode and IsBlockNode take nsINode (GetParent would return null for attributes and documents). I would also make GetBlockParent take a nsIContent and do the QI in the one caller.
I'm not trying to convert everything to use nsINode/Content, just fixing some
obvious problems, mainly GetBlockParent.

If SkipNode/IsBlockNode took nsINode*, they would have to have similar
nsINode* to nsIContent* casting that in the patch is done in one place, in
GetCurrentContent().

Not sure why the caller should QI, not GetBlockParent. That shouldn't matter at all. There is anyway one QI, unless someone does bigger changes and makes
mIternode to be nsINode* or nsIContent*.

Anyway, the patch is just a fix-something-while-quickly-glancing-over-some-unowned-code-during-a-train-trip :)
With this the AddRef/Release caused by nsFind drops down from the
top of the Shark profile. Testcase is to search for some non-existing word
in mxr.m.o/nsCSSFrameConstructor.cpp.
Attachment #349581 - Attachment is obsolete: true
Attachment #349734 - Flags: superreview?(peterv)
Attachment #349734 - Flags: review?(peterv)
Attachment #349581 - Flags: superreview?(peterv)
Attachment #349581 - Flags: review?(peterv)
Maybe it's the case to ask a different reviewer...
The patch does seem to apply cleanly (with --fuzz=4).
Peter, just say if you don't like the approach.
Peter, do you think you'll review this patch in the near future? I'll help refresh the patch if Olli's approach is fine. Thanks.
Hardware: x86 → All
Attachment #349734 - Flags: superreview?(peterv)
Attachment #349734 - Flags: review?(peterv)
Whiteboard: [feature] p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: [feature] p=0 → p=0
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: