493701 - Use TObserverArray for DocLoader Listeners

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Arpad Borsos [:Swatinem]]

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();

Comment 1

[Boris Zbarsky [:bzbarsky]]

Arpad, do you want to take this?

Comment 2

[Arpad Borsos [:Swatinem]]

Yes, I take this once the original patch has re-landed.

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: patch [pushed: comment 5]

This patch also adds the tests suggested in bug 474369 comment 155.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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 5

[Arpad Borsos [:Swatinem]]

Comment on attachment 382682 [details] [diff] [review]
patch [pushed: comment 5]

Pushed as http://hg.mozilla.org/mozilla-central/rev/2f2586790f32 with nits fixed.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

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)

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's more likely to be the other than this one; this bug shouldn't change what the compiled code looks like.

Comment 8

[Arpad Borsos [:Swatinem]]

I backed this out together with bug 474369 for performance regressions.

Comment 9

[Arpad Borsos [:Swatinem]]

Attachment: patch, v2

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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Want to try to reland this?

Comment 11

[Arpad Borsos [:Swatinem]]

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.

Comment 12

[Arpad Borsos [:Swatinem]]

Attachment: unbitrotted

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.

Comment 13

[Arpad Borsos [:Swatinem]]

Attachment: part 1: add nsTObserverArray::BackwardIterator

Comment 14

[Arpad Borsos [:Swatinem]]

Attachment: part2: Use TObserverArray for DocLoader Listeners

Wow, lets resurrect some 4 year old zombie.

I would feel more comfortable if you could review again :-)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8406816 [details] [diff] [review]
part 1: add nsTObserverArray::BackwardIterator

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Arpad Borsos [:Swatinem]]

(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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Makes sense to me.

Comment 19

[Arpad Borsos [:Swatinem]]

Attachment: iteratorremove.patch

Alright, here it is. I will probably fold this into the first 2 patches for landing.

Comment 20

[Boris Zbarsky [: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...

Comment 21

[Arpad Borsos [:Swatinem]]

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

Comment 22

[Nathan Froyd [:froydnj]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/73055a66d95a for across-the-board Cpp bustage, e.g.:

https://tbpl.mozilla.org/php/getParsedLog.php?id=38558037&tree=Mozilla-Inbound

Comment 23

[Arpad Borsos [:Swatinem]]

   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.

Comment 24

[Arpad Borsos [:Swatinem]]

(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.

Comment 25

[Arpad Borsos [:Swatinem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff4e97827d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f8577510ea

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bff4e97827d2
https://hg.mozilla.org/mozilla-central/rev/30f8577510ea