Closed
Bug 1123151
Opened 10 years ago
Closed 10 years ago
Privatize PLDHashTable::ops
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(3 files)
25.16 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
69.32 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
17.38 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
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.
Attachment #8551067 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 2•10 years ago
|
||
This encapsulates most of the uses of PLDHashTable::ops.
Attachment #8551068 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 3•10 years ago
|
||
This required adding a getter and a setter, but they're used sparingly.
Attachment #8551069 -
Flags: review?(nfroyd)
![]() |
||
Comment 4•10 years ago
|
||
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.
Attachment #8551067 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•10 years ago
|
Attachment #8551068 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 5•10 years ago
|
||
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.
Attachment #8551069 -
Flags: review?(nfroyd) → review+
![]() |
Assignee | |
Comment 6•10 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 7•10 years ago
|
||
![]() |
Assignee | |
Comment 8•10 years ago
|
||
![]() |
Assignee | |
Comment 9•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ad73fa3f416
https://hg.mozilla.org/mozilla-central/rev/694f5026338d
https://hg.mozilla.org/mozilla-central/rev/35d043b3e889
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
![]() |
Assignee | |
Updated•10 years ago
|
Blocks: modernize-pldhash
You need to log in
before you can comment on or make changes to this bug.
Description
•