1186780 - Use some new hashtable iterators

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: ImageLoader

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: DOM

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Cycle collection

Comment 4

[Boris Zbarsky [:bzbarsky]]

I might not get to this review this week; I have a huge review backlog already.  :(

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8637720 [details] [diff] [review]
DOM

Moving down the list of people who start with b ...

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8637721 [details] [diff] [review]
Cycle collection

Review of attachment 8637721 [details] [diff] [review]:
-----------------------------------------------------------------

nice

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8637720 [details] [diff] [review]
DOM

Review of attachment 8637720 [details] [diff] [review]:
-----------------------------------------------------------------

I must admit the diff algorithm gave some hard to read hunks for the macro changes in nsDocument.cpp, but I think this is correct.  Just a couple questions and nit.

Also, please remember to update the bug number and reviewer in the commit message.

Thanks.

::: dom/base/nsDocument.cpp
@@ +2034,5 @@
>    if (tmp->mAnimationController) {
>      tmp->mAnimationController->Unlink();
>    }
>  
> +  tmp->MaybeClearBoxObjectTable();

Any particular reason you moved clearing the box table further down in the steps here?  This used to take place after:

  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPreloadingImages)

@@ +3779,5 @@
> +    return;
> +  }
> +
> +  for (auto iter = mBoxObjectTable->ConstIter(); !iter.Done(); iter.Next()) {
> +    if (auto boxObject = iter.UserData()) {

nit: I personally prefer to see the variable declaration on a separate line. Assignment in an if-statement seems to favor cleverness over clarity. Here and elsewhere in the patch.

This is a style thing, though, so I won't make you change it if you greatly prefer this.

@@ +10607,5 @@
>  
>    // Otherwise, iterate over our images and perform the appropriate action.
> +  for (auto iter = mImageTracker.ConstIter(); !iter.Done(); iter.Next()) {
> +    imgIRequest* image = iter.Key();
> +    if (aLocked) {

Are these methods particularly hot?  It seems you've moved an if-check inside a for loop which could have some perf impact.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ben Kelly [:bkelly] from comment #7)
> Any particular reason you moved clearing the box table further down in the
> steps here?  This used to take place after:

Nope, fixed.

> nit: I personally prefer to see the variable declaration on a separate line.
> Assignment in an if-statement seems to favor cleverness over clarity. Here
> and elsewhere in the patch.
>
> This is a style thing, though, so I won't make you change it if you greatly
> prefer this.

I really like that style, so I've left it as is.

> Are these methods particularly hot?  It seems you've moved an if-check
> inside a for loop which could have some perf impact.

Excellent question.  These functions are called when the presshell is frozen or thawed, or when the active state of the docshell changes.  None of those should happen frequently, and none of them are performance sensitive to the level at which a branch would affect them.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca5723f69d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e006947db44

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f388b1612356
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1df3bc0b4fd

Comment 11

[Carsten Book [:Tomcat]]

this caused massive test bustages due to crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=12136520&repo=mozilla-inbound

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The problem is in the DOM patch, so I've relanded the cycle collector one.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0edab7173ae

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0edab7173ae

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8637719 [details] [diff] [review]
ImageLoader

I wonder if this pattern (twice) is really needed:

>+    auto key = iter.Key();
>+    imgIRequest* request = static_cast<imgIRequest*>(key);

>+    auto key = iter.Key();
>+    imgIRequest* request = static_cast<imgIRequest*>(key);


Otherwise, r=dbaron

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43ae47ffe011

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9012f078265

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43ae47ffe011
https://hg.mozilla.org/mozilla-central/rev/e9012f078265