Closed Bug 493701 Opened 11 years ago Closed 6 years ago

Use TObserverArray for DocLoader Listeners

Categories

(Core :: DOM: Navigation, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: Swatinem, Assigned: Swatinem)

References

Details

Attachments

(3 files, 3 obsolete files)

Per bug 474369 comment 140:
It might be nice to have a macro that takes a flag-set and some code and then
basically does the whole "iterate the array, skip infos that don't have these
flags, call this code on the others" thing.  That should be a followup bug,
though.

The code in question looks like this:

  // First notify any listeners of the new state info...
  nsCOMPtr<nsIWebProgressListener> listener;
  nsTObserverArray<nsListenerInfo>::EndLimitedIterator iter(mListenerInfoList);

  while (iter.HasMore()) {
    nsListenerInfo &info = iter.GetNext();
    if (!(info.mNotifyMask & _FLAG_)) {
      continue;
    }

    listener = do_QueryReferent(info.mWeakListener);
    if (!listener) {
      // the listener went away. gracefully pull it out of the list.
      mListenerInfoList.RemoveElementAt(&info - mListenerInfoList.Elements());
      continue;
    }

    listener->_CODE_;
  }
  mListenerInfoList.Compact();
Arpad, do you want to take this?
Yes, I take this once the original patch has re-landed.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Attached patch patch [pushed: comment 5] (obsolete) — Splinter Review
This patch also adds the tests suggested in bug 474369 comment 155.
Attachment #382682 - Flags: review?(bzbarsky)
Attachment #382682 - Flags: review?(bzbarsky) → review+
Comment on attachment 382682 [details] [diff] [review]
patch [pushed: comment 5]

1) Use PR_BEGIN_MACRO and PR_END_MACRO instead of do/while (it's the same in the end, but clearer as to why it's there).

2)  Put parens around _flags in the macro body.

With those nits, looks good.
Comment on attachment 382682 [details] [diff] [review]
patch [pushed: comment 5]

Pushed as http://hg.mozilla.org/mozilla-central/rev/2f2586790f32 with nits fixed.
Attachment #382682 - Attachment description: patch → patch [pushed: comment 5]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Regression: DHTML increase 0.98% on XP Firefox:
http://graphs-new.mozilla.org/graph.html#tests=[{%22machine%22:55,%22test%22:18,%22branch%22:1},{%22machine%22:56,%22test%22:18,%22branch%22:1},{%22machine%22:57,%22test%22:18,%22branch%22:1},{%22machine%22:71,%22test%22:18,%22branch%22:1},{%22machine%22:108,%22test%22:18,%22branch%22:1},{%22machine%22:109,%22test%22:18,%22branch%22:1},{%22machine%22:110,%22test%22:18,%22branch%22:1},{%22machine%22:111,%22test%22:18,%22branch%22:1}]&sel=1244728380,1244901180

Previous results: 1083.47 from build 20090612062022 of revision 04ed36482fa9 at 2009-06-12 06:20:00 on qm-pxp-fast03 run # 0

New results: 1094.12 from build 20090612065328 of revision 2f2586790f32 at 2009-06-12 06:53:00 on qm-pxp-trunk01 run # 0

(this and bug 474369 fall into the suspected regression range)
It's more likely to be the other than this one; this bug shouldn't change what the compiled code looks like.
I backed this out together with bug 474369 for performance regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Summary: Use a macro to reduce redundancy in nsDocLoader.cpp → Use TObserverArray for DocLoader Listeners
Attached patch patch, v2 (obsolete) — Splinter Review
I split out the related changes from bug 474369, forwarding r=bz, as you have also reviewed the docloader changes from the other bug.
Status report is following in the other bug as well.
Attachment #382682 - Attachment is obsolete: true
Attachment #385614 - Flags: review+
I'm on vacation starting tomorrow so I won't be able to watch the tree. Also I'm not sure what happened to the dhtml regressions.
Attached patch unbitrotted (obsolete) — Splinter Review
Long time no progress here. I folded parts of bug 474369 into this one, so the two patches are not dependent any more.
Otherwise its just minor changes unbitrotting the patch, I won’t bother you with requesting review again.
Attachment #385614 - Attachment is obsolete: true
Attachment #466290 - Flags: review+
Wow, lets resurrect some 4 year old zombie.

I would feel more comfortable if you could review again :-)
Attachment #466290 - Attachment is obsolete: true
Attachment #8406817 - Flags: review?(bzbarsky)
Comment on attachment 8406816 [details] [diff] [review]
part 1: add nsTObserverArray::BackwardIterator

r=me
Attachment #8406816 - Flags: review?(bzbarsky) → review+
Comment on attachment 8406817 [details] [diff] [review]
part2: Use TObserverArray for DocLoader Listeners

s/PRBool/bool/, please.

I worry a bit about quadratic behavior from RemoveEmptyListeners...  maybe it's ok.  And maybe call it RemoveDeadListeners?

r=me
Attachment #8406817 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #16)
> I worry a bit about quadratic behavior from RemoveEmptyListeners...  maybe
> it's ok.  And maybe call it RemoveDeadListeners?

Reading my own 5 year old code, the method is only there because BackwardIterator does not expose the current index, and has no Remove() method.
How about adding such a method instead?
Makes sense to me.
Alright, here it is. I will probably fold this into the first 2 patches for landing.
Attachment #8409289 - Flags: review?(bzbarsky)
Comment on attachment 8409289 [details] [diff] [review]
iteratorremove.patch

Please document that calling Remove() won't affect what the ext GetNext() call returns.

r=me, and sorry for the lag...
Attachment #8409289 - Flags: review?(bzbarsky) → review+
Comment added. Also, I folded the 3rd patch into the first two, like I said.
Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/154fb9a9e0bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f615a0532971
   static int test27Expected[] = { 1, 8, 2, 7, 4, 9, 3 };
[…]
-  static int test28Expected[] = { 1, 8, 2, 7, 4, 3 };
+  static int test28Expected[] = { 1, 2, 7, 4, 9, 3 };

oh boy, since the expected is in iteration order, ofc the 2nd element from the front is removed.

I will reland once I get the tests to run locally, having some problem with that.
(In reply to Arpad Borsos (Swatinem) from comment #23)
>    static int test27Expected[] = { 1, 8, 2, 7, 4, 9, 3 };
> […]
> -  static int test28Expected[] = { 1, 8, 2, 7, 4, 3 };
> +  static int test28Expected[] = { 1, 2, 7, 4, 9, 3 };
> 
> oh boy, since the expected is in iteration order, ofc the 2nd element from
> the front is removed.

   // Removal using Iterator
   DO_TEST(BackwardIterator, test27Expected,
+          // when this code runs, |GetNext()| has only been called once, so
+          // this actually removes the very first element
           if (count == 1) iter.Remove();
           );
 
-  static int test28Expected[] = { 1, 2, 7, 4, 9, 3 };
+  static int test28Expected[] = { 8, 2, 7, 4, 9, 3 };

So it is actually the first element, since |GetNext()| is called after _code. Sorry for the churn.
https://hg.mozilla.org/mozilla-central/rev/bff4e97827d2
https://hg.mozilla.org/mozilla-central/rev/30f8577510ea
Status: ASSIGNED → RESOLVED
Closed: 11 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.