466300 - Reduce QI and AddRef/Release in nsFind.cpp

Status: NEW

(Core :: Find Backend, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this should help a bit

Comment 2

[Peter Van der Beken [: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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: mIterContent for faster iteration

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.

Comment 5

[mmc]

Maybe it's the case to ask a different reviewer...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

The patch does seem to apply cleanly (with --fuzz=4).
Peter, just say if you don't like the approach.

Comment 7

[Matthew N. [:MattN]]

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.