make search listener registration optionally only get start/end notifications

RESOLVED FIXED

Status

MailNews Core
Search
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

({perf})

Trunk
x86
Windows 7

Firefox Tracking Flags

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
if a listener is only interested in start/end notifications, then sending onSearchHit notifications is a needless expense, especially if the listener is js.
(Assignee)

Comment 1

8 years ago
Created attachment 440398 [details] [diff] [review]
wip

this should work - I had to edit the patch a bit to separate it from the other view stuff, so I may have messed up...
(Assignee)

Updated

8 years ago
Attachment #440398 - Flags: review?(bugmail)

Updated

8 years ago
Keywords: perf
the age old problem of listeners that remove themselves is rearing its ugly head.  nsTObserverArray is of course clever enough to deal with this, but not clever enough to help us out; there are no additional slots supported, no exposed current position, and no overloaded arithmetic to expose the current position.

I fear it makes the most sense to support a single static integer we use as an iterator offset that the manipulation functions adjust as appropriate (and ignore when -1) given that the code is non re-entrant.  ridonculous.  The end-of-work-day steam whistle is blowing though, so I'll do that tomorrow if the bug does not grow a better suggestion before I get in tomorrow morn.
(Assignee)

Comment 3

8 years ago
Ah, listeners are removing themselves during the onSearchDone notification, messing up our iteration, or rather, our index into the notification flags? Can we get the index of the next listener (if any) before we send the notification, and check it after we send the notification, and if it has decreased, decrease our local flag index by the same? Generally, it would have decreased by 1. I suppose that doesn't deal with new listeners getting registered from the onSearchDone notification, but that's not going to happen...
the nsTObserverArray iterator doesn't expose its state apart from implementing less-than.  If we want to assume that the only change that would occur is removal of the listener we are actively processing (with no offsetting newly added listener and no other listeners removed), then we can just check for a change in the size of the listener list and adjust accordingly...

For the actual use-cases we currently have that I am aware of, that should be safe, but it does not keep us safe from jerks.  I'm fine with it if you're fine with it :)
(Assignee)

Comment 5

8 years ago
I'm ok with it, though I can imagine multiple listeners getting removed if there's some goofy ownership relationship between listeners. In which case, the static iterator offset integer would be OK. We might as well do the latter, since it's pretty much just as simple...
Okay, I'll do that then, noting that I used the wrong term when I said 'static'.  I meant per-session instance; the cop-out is that we aren't going to handle a single session that somehow has multiple concurrent iterations happening.  (The pathological case that would likely spawn that would be a listener attempting to cancel an ongoing search and replace it from a new one in a naive/dangerous way; in that case the inner iteration would basically terminate the outer one, which is desirable.)
Created attachment 440613 [details] [diff] [review]
modification bacon

I think these are my changes on top of your changes to address the situation as proposed.
Attachment #440613 - Flags: review?(bienvenu)
(Assignee)

Comment 8

8 years ago
Comment on attachment 440613 [details] [diff] [review]
modification bacon

looks reasonable...
Attachment #440613 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 9

8 years ago
this was checked in with bug 545955
Status: NEW → RESOLVED
Last Resolved: 8 years ago
status-thunderbird3.1: --- → beta2-fixed
Resolution: --- → FIXED
Attachment #440398 - Attachment is obsolete: true
Attachment #440398 - Flags: review?(bugmail)
You need to log in before you can comment on or make changes to this bug.