1174631 - Remove PL_DHashTableEnumerate() use from layout/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nicholas Nethercote [inactive]]

Because PLDHashTable::Iter is nicer than PL_DHashTableEnumerate().

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove PL_DHashTableEnumerate uses from nsRuleNode

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove PL_DHashTableEnumerate uses from nsCSSRuleProcessor

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove PL_DHashTableEnumerate uses from nsCSSRuleProcessor

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove PL_DHashTableEnumerate uses from SpanningCellSorter

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove PL_DHashTableEnumerate uses from nsFrameManager

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8622318 [details] [diff] [review]
(part 1) - Remove PL_DHashTableEnumerate uses from nsRuleNode

># HG changeset patch
># User Nicholas Nethercote <[e-mail]>
># Date 1433988795 25200
>#      Wed Jun 10 19:13:15 2015 -0700
># Node ID 3c2920df665fd11f20d588646ce1ec00af89b401
># Parent  990bb74f2c755163f0b63269144eb56b11f447a8
>Bug 1174631 (part 1) - Remove PL_DHashTableEnumerate uses from nsRuleNode. r=dholbert.

Nit: the commit message, "remove PL_DHashTableEnumerate", only tells half of the story here.

I'd prefer that we mention the replacement (PLDHashTable::Iter) as well, e.g.:
 Replace PL_DHashTableEnumerate uses with PLDHashTable::Iter, in nsRuleNode.
or:
 Replace nsRuleNode's uses of PL_DHashTableEnumerate with PLDHashTable::Iter.

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

>-    PL_DHashTableEnumerate(children, EnqueueRuleNodeChildren,
>-                           &destroyQueueTail);
>+    nsRuleNode ***destroyQueueTailPtr = &destroyQueueTail;
>+    for (PLDHashTable::Iter iter(children); !iter.Done(); iter.Next()) {
>+      auto entry = static_cast<ChildrenHashEntry*>(iter.Get());
>+      **destroyQueueTailPtr = entry->mRuleNode;
>+      *destroyQueueTailPtr = &entry->mRuleNode->mNextSibling;

I think destroyQueueTailPtr can be removed here -- it's based on an argument to a method that no longer exists, and it seems like needless abstraction now.

Can we just do s/*destroyQueueTailPtr/destroyQueueTail/ for these last 2 lines?

>-    PL_DHashTableEnumerate(children, SweepHashEntry, &survivorsWithChildren);
>+    nsRuleNode** survivorsWithChildrenPtr = &survivorsWithChildren;
>+    for (PLDHashTable::RIter iter(children); !iter.Done(); iter.Next()) {

Any reason we're using RIter instead of just Iter here? (Looks like we were iterating in the forwards direction before?)

Maybe it's because we're calling Remove()?  If we have to do reverse iteration in order to remove() (not sure), please add a comment before the loop to indicate that.

>+      auto entry = static_cast<ChildrenHashEntry*>(iter.Get());
>+      nsRuleNode* node = entry->mRuleNode;
>+      if (node->DestroyIfNotMarked()) {
>+        iter.Remove();
>+      } else if (node->HaveChildren()) {
>+        // When children are hashed mNextSibling is not normally used but we
>+        // use it here to build a list of children that needs to be swept.
>+        nsRuleNode** headQ = survivorsWithChildrenPtr;

As above, there's no reason for survivorsWithChildrenPtr to exist here. Just directly use "&survivorsWithChildren", and drop the Ptr variable.

Comment 7

[Daniel Holbert [:dholbert]]

(I can't actually build with these patches applied; it looks like these iter types are being added in bug 1174625. Adding dependency.)

Comment 8

[Daniel Holbert [:dholbert]]

While we're creating these new iter types, maybe it'd be worth making your new iterator types support range-based for loops?  Then we can jump straight to iterating like so:
  for (PLDHashTable::Iter iter : children) {
...instead of the longer & harder-to-read:
  for (PLDHashTable::Iter iter(children); !iter.Done(); iter.Next()) {

This would mean a bit of extra work up-front, but I imagine it'll make the new code here & in other bugs where you intend to use these iterators a bit easier to write & review.

(I haven't actually converted any classes to support range-based for loops, so I'm not sure how hard it is.  But for reference, xidorn just modernized nsFrameList to support range-based for loops in bug 1143513.  I suspect he could help if you run into trouble, and you could also probably crib from that bug.)

If you'd rather punt on that and make these loops a 2-step conversion, that's fine with me too.  (But if we're eventually going to have to do some rewriting/rework to make these types support range-based syntax, it might be better to just do that up-front.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8622321 [details] [diff] [review]
(part 2) - Remove PL_DHashTableEnumerate uses from nsCSSRuleProcessor

(The commit message nit from comment 6 applies to the rest of the patches too.)

>diff --git a/layout/style/nsCSSRuleProcessor.cpp b/layout/style/nsCSSRuleProcessor.cpp
>-      PL_DHashTableEnumerate(&data.mRulesByWeight, FillWeightArray, &fwData);
>+      int32_t j = 0;
>+      for (PLDHashTable::Iter i(&data.mRulesByWeight); !i.Done(); i.Next()) {
>+        auto entry = static_cast<const RuleByWeightEntry*>(i.Get());
>+        weightArray[j++] = entry->data;
>+      }

When I see "i"/"j" being used side-by-side in a loop, I assume they're both numeric loop-counters of some sort. But here, that's not true for either variable; one is a non-numeric iterator, and the other is a numeric index which isn't counting loops.

I'd prefer:
 - name "j" something meaningful, e.g. weightIdx
 - and maybe consider renaming "i" to "iter", though "i" is probably also fine once "j" has been renamed.

r=me with that

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> When I see "i"/"j" being used side-by-side in a loop, [more blathering]

Sorry, feel free to disregard this point; I mistakenly thought there was some condition around the 'j' usage. But there is no such condition, so it does effectively end up being a loop counter, since it gets incremented on each loop iteration. So, it's less confusing than I thought.  (This may still benefit from a rename -- particularly s/i/iter/ to avoid implying the standard "i/j nested-loop over 2 arrays" pattern -- but no need if you prefer the naming as it stands.)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8622322 [details] [diff] [review]
(part 3) - Remove PL_DHashTableEnumerate uses from SpanningCellSorter

>+++ b/layout/tables/SpanningCellSorter.cpp
>+                int32_t j = 0;
>+                for (PLDHashTable::Iter i(&mHashTable); !i.Done(); i.Next()) {
>+                    sh[j++] = static_cast<HashTableEntry*>(i.Get());
>+                }

As above: consider renaming "i" or "j" here, to avoid hinting that there's some sort of abstract nested for loop with two independent loop counters.

r=me regardless

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8622323 [details] [diff] [review]
(part 4) - Remove PL_DHashTableEnumerate uses from nsFrameManager

r=me on part 4

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> >-    PL_DHashTableEnumerate(children, SweepHashEntry, &survivorsWithChildren);
> >+    nsRuleNode** survivorsWithChildrenPtr = &survivorsWithChildren;
> >+    for (PLDHashTable::RIter iter(children); !iter.Done(); iter.Next()) {
> 
> Any reason we're using RIter instead of just Iter here? (Looks like we were
> iterating in the forwards direction before?)
> 
> Maybe it's because we're calling Remove()?  If we have to do reverse
> iteration in order to remove() (not sure), please add a comment before the
> loop to indicate that.

Ohhh - so, I immediately assumed RIter meant "reverse iterator" (though in retrospect, "reverse" maybe doesn't make much sense for hashtable iteration).   Looking at bug 1174625, I see now that "R" really stands for "removing", not "reverse", and you've got a proposal to rename the type to make this clearer. Cool; never mind about this review comment then.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> While we're creating these new iter types, maybe it'd be worth making your
> new iterator types support range-based for loops?  Then we can jump straight
> to iterating like so:
>   for (PLDHashTable::Iter iter : children) {
> ...instead of the longer & harder-to-read:
>   for (PLDHashTable::Iter iter(children); !iter.Done(); iter.Next()) {
> 
> This would mean a bit of extra work up-front, but I imagine it'll make the
> new code here & in other bugs where you intend to use these iterators a bit
> easier to write & review.
> 
> (I haven't actually converted any classes to support range-based for loops,
> so I'm not sure how hard it is.  But for reference, xidorn just modernized
> nsFrameList to support range-based for loops in bug 1143513.  I suspect he
> could help if you run into trouble, and you could also probably crib from
> that bug.)
> 
> If you'd rather punt on that and make these loops a 2-step conversion,
> that's fine with me too.  (But if we're eventually going to have to do some
> rewriting/rework to make these types support range-based syntax, it might be
> better to just do that up-front.)

I remember heycam expressed the same hope that we should make the hash table support range-based for loop. Because of that, I filed bug 1149833 and listed some difficulties there.

Comment 15

[Nicholas Nethercote [inactive]]

> Ohhh - so, I immediately assumed RIter meant "reverse iterator"

Yes! I wrote a comment about this yesterday but Bugzilla seems to have swallowed it. I probably got a conflict that I didn't see.

As for the range-based iterator, I'd rather do that as a follow-up. IMO the change from PL_DHashTableEnumerator to an iterator is a large improvement (no more function pointers and void* arguments, thank heavens) whereas the change from a normal iterator to a range-based one is a small improvement. And I'm already half-way through the conversions to vanilla iterators, so I might as well continue in the same vein.

Comment 16

[Nicholas Nethercote [inactive]]

> > When I see "i"/"j" being used side-by-side in a loop, [more blathering]

I have been using |iter| for iterator names *except* when it would make the for-loop head more than 80 chars, whereupon I would switch to |i|. But with the changes in bug 1174625 making things more compact I now have the following, which does fit in 80 chars:

> for (auto iter = data.mRulesByWeight.Iter(); !iter.Done(); iter.Next()) {

So that's good.

As for the |j|, there are other |i| variables in that function and I get warnings about shadowing if I use |i|. It would be better if I could restrict the scope of |j| to the for-loop but you can't declare multiple variables in a for-loop if they have different types.

Another possibility would be to add a method to the iterator that returns the current element number, but I decided against that since it's needed so rarely.

Anyway, I'm just letting you know that I did give some thought to all of this :)

Comment 17

[Daniel Holbert [:dholbert]]

Thanks! That all makes sense.

I'd like to see the updated "part 1" before granting r+ there -- worth a brief review sanity-check on my suggestions there, since we've got multiple levels of pointer-dereferencing, and it might be easy to make a mistake.

After that, this will be good to go I think. (pending any other bugs like bug 1173600 which you want to layer this on top of)

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace nsRuleNode's uses of PL_DHashTableEnumerate() with PLDHashTable::{,Removing}Iterator

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8623430 [details] [diff] [review]
(part 1) - Replace nsRuleNode's uses of PL_DHashTableEnumerate() with PLDHashTable::{,Removing}Iterator

Looks good, thanks!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b94564503734
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7f94db9f81
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2f43991ab4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a5771528a4

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b94564503734
https://hg.mozilla.org/mozilla-central/rev/2c7f94db9f81
https://hg.mozilla.org/mozilla-central/rev/aa2f43991ab4
https://hg.mozilla.org/mozilla-central/rev/b1a5771528a4