1123151 - Privatize PLDHashTable::ops

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

PLDHashTable::ops is public, and frequently used to indicate if a PLDHashTable
has been initialized. It can be made private and we can make PLDHashTable nicer
to use in the process.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 1) - Set PLDHashTable::ops consistently

Currently the setting of PLDHashTable::ops is very haphazard.

- PLDHashTable has no constructor, so it's not auto-nulled, so lots of places
  null it themselves.

- In the fallible PLDHashTable::Init() function, if the entry storage
  allocation fails we'll be left with a table that has |ops| set -- indicating
  it's been initialized -- but has null entry storage. I'm not certain this can
  cause problems but it feels unsafe, and some (but not all) callers of Init()
  null it on failure.

- PLDHashTable does not null |ops| in Finish(), so some (but not all) callers
  do this themselves.

This patch makes things simpler.

- It adds a constructor that zeroes |ops|.

- It modifies Init() so that it only sets |ops| once success is ensured.

- It zeroes |ops| in Finish().

- Finally, it removes all the now-unnecessary |ops| nulling done by the users
  of PLDHashTable.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Add PLDHashTable::IsInitialized()

This encapsulates most of the uses of PLDHashTable::ops.

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 3) - Make PLDHashTable::ops private

This required adding a getter and a setter, but they're used sparingly.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8551067 [details] [diff] [review]
(part 1) - Set PLDHashTable::ops consistently

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

It'd be good to add checks in xpcom/tests/TestPLDHash.cpp for:

- The constructor zero-ing |ops|.
- |ops| not being set if |Init()| returns failure.
- |Finish()| zero-ing |ops| appropriately.

::: layout/style/nsCSSRuleProcessor.cpp
@@ +3341,5 @@
>        mCacheKey(aKey),
>        mSheetType(aSheetType)
>    {
> +    unused << PL_DHashTableInit(&mRulesByWeight, &gRulesByWeightOps,
> +                                sizeof(RuleByWeightEntry), fallible_t(), 32);

This is an odd construction; I think it's worth a comment as to why we're doing this.  Alternatively, you could make this infallible.

I don't have the experience to say what a good comment is here, or the repercussions of making this infallible.  Ask heycam for review on those?

::: xpcom/glue/pldhash.h
@@ +223,5 @@
>  
>  public:
> +  // All the other fields are initialized in Init(), but we zero |ops| here
> +  // because it's used to determine if Init() has been called.
> +  PLDHashTable() : ops(nullptr) {}

MOZ_CONSTEXPR this constructor, please.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8551069 [details] [diff] [review]
(part 3) - Make PLDHashTable::ops private

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +1959,5 @@
>    if (entry->mNpp == nppcx->npp) {
>      // Prevent invalidate() and deallocate() from touching the hash
>      // we're enumerating.
> +    const PLDHashTableOps *ops = table->Ops();
> +    table->SetOps(nullptr);

This is...gross.

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> It'd be good to add checks in xpcom/tests/TestPLDHash.cpp

Good idea.

> > +    unused << PL_DHashTableInit(&mRulesByWeight, &gRulesByWeightOps,
> > +                                sizeof(RuleByWeightEntry), fallible_t(), 32);
> 
> This is an odd construction; I think it's worth a comment as to why we're
> doing this.  Alternatively, you could make this infallible.
> 
> I don't have the experience to say what a good comment is here, or the
> repercussions of making this infallible.  Ask heycam for review on those?

heycam said making it infallible is fine. The number of entries is small, and each entry is only a few words.

> > +  // All the other fields are initialized in Init(), but we zero |ops| here
> > +  // because it's used to determine if Init() has been called.
> > +  PLDHashTable() : ops(nullptr) {}
> 
> MOZ_CONSTEXPR this constructor, please.

I ended up having to zero every member (constexpr constructor require this) but that's ok.

> This is...gross.

Indeed. I won't pretend to understand if it's necessary; I preserved the functionality and otherwise left it alone.

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd064f0e28d9

Comment 8

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad73fa3f416
https://hg.mozilla.org/integration/mozilla-inbound/rev/694f5026338d
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d043b3e889

Comment 9

[Nicholas Nethercote [:njn] (On Long Service Leave)]

FWIW, these are all common patterns:

> if (t.IsInitialized()) { PL_DHashTableFinish(t); }
>
> n += t.IsInitialized() ? t.SizeOf{In,Ex}cludingThis(t, ...) : 0;
>
> if (!t.IsInitialized()) { PL_DHashTableInit(t, ...); }
>
> if (t.IsInitialized()) { PL_DHashTableEnumerate(t, ...); }

I considered changing the PLDHashTable functions so that you could just call them and they would do nothing if that was appropriate (or return zero, in the *SizeOf* case). It would get rid of many IsInitialized() calls.

But in the end I didn't, because the other PLDHashTable functions (like Add()) don't really have a sensible no-op, and because this would introduce an inconsistency with js::HashTable.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ad73fa3f416
https://hg.mozilla.org/mozilla-central/rev/694f5026338d
https://hg.mozilla.org/mozilla-central/rev/35d043b3e889