Closed
Bug 1123151
Opened 9 years ago
Closed 9 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•9 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•9 years ago
|
||
This encapsulates most of the uses of PLDHashTable::ops.
Attachment #8551068 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•9 years ago
|
||
This required adding a getter and a setter, but they're used sparingly.
Attachment #8551069 -
Flags: review?(nfroyd)
Comment 4•9 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•9 years ago
|
Attachment #8551068 -
Flags: review?(nfroyd) → review+
Comment 5•9 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•9 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•9 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd064f0e28d9
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 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•9 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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Updated•9 years ago
|
Blocks: modernize-pldhash
You need to log in
before you can comment on or make changes to this bug.
Description
•