All users were logged out of Bugzilla on October 13th, 2018

Privatize most of PL_DHashTable's fields

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
PL_DHashTable is a completely public struct. It shouldn't be.
(Assignee)

Updated

4 years ago
Depends on: 1057914
(Assignee)

Comment 1

4 years ago
|ops| will need to remain public. Lots of places use this to indicate if a table is live -- both reading and writing it -- so making it private would be pointless because we'd need both a getter and a setter.
(Assignee)

Updated

4 years ago
Depends on: 1057928
(Assignee)

Updated

4 years ago
Blocks: 1058335
(Assignee)

Comment 2

4 years ago
Created attachment 8478749 [details] [diff] [review]
(part 1) - Make prefapi.cpp's |gHashTable| static

|gHashTable| in prefapi.cpp is not static, because it's used also in
nsPrefBranch.cpp and Preferences.cpp. So it's explicitly zeroed at its
definition. But this blocks the privatization of PLDHashTable's fields -- the
explicit zeroing sets the would-be-private fields. And I don't want to add any
constructors to PLDHashTable, because there are lots of static PLDHashTables.

So I've made a wrapper class, |PrefsTable|, that contains just a single static
PLDHashTable, |mTable|, which replaces the old |gHashTable|. That gives me a
hash table that is both static *and* visible in multiple .cpp files.
Attachment #8478749 - Flags: review?(roc)
(Assignee)

Comment 3

4 years ago
Created attachment 8478750 [details] [diff] [review]
(part 2) - Privatize most of PLDHashTable's fields

This patch privatizes most of the members of of PLDHashTable.

- |ops| is left public, because lots of places null it to indicate if a table
  is no longer live.
  
- |data| is left public, because it's the play-thing of PLDHashTable's users.

- All the PL_DHashTableFoo() functions are left in place, to minimize churn
  among PLDHashTable's users. Many of these now immediately call into methods,
  e.g. PL_DHashTableFinish() calls PLDHashTable::Finish(). These new methods
  are all marked MOZ_ALWAYS_INLINE within pldhash.cpp so the "call onto" step
  has no cost, but I could be convinced to do otherwise.

- nsAtomTable.cpp zeroes |entryCount| in one place, at the same time it zeroes
  |ops|. I've removed this, and adjusted accordingly the place where
  |entryCount| is checked.

- Some declarations had to be moved earlier in pldhash.h.

- I removed PL_DHASH_ENTRY_IS_LIVE, because it was no longer needed.

There are numerous uses of PL_DHASH_FASTCALL in this file. I'm skeptical that
they have any effect, but I've left them in for now.
Attachment #8478750 - Flags: review?(roc)
Comment on attachment 8478749 [details] [diff] [review]
(part 1) - Make prefapi.cpp's |gHashTable| static

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

I'm a bit confused by this patch. Why can't you leave the definition of gHashTable as-is and just not give it an explicit initializer?
Attachment #8478749 - Flags: review?(roc) → review-
(Assignee)

Comment 5

4 years ago
> I'm a bit confused by this patch. Why can't you leave the definition of
> gHashTable as-is and just not give it an explicit initializer?

Because it's not static, it won't be auto-zeroed[*]. And if it's not auto-zeroed, the |ops| checks that are used to determine if it's been instantiated won't be safe.

[*] Actually, I'm now not certain if that is true.
(Assignee)

Comment 6

4 years ago
http://stackoverflow.com/questions/1597405/what-happens-to-a-declared-uninitialized-variable-in-c-does-it-have-a-value says that global variables are always zeroed, even if they're not static. So I can just remove the explicitly initialization of |gHashTable| like you suggest.
(Assignee)

Comment 7

4 years ago
> These new methods are all marked MOZ_ALWAYS_INLINE within pldhash.cpp so the
> "call onto" step has no cost, but I could be convinced to do otherwise.

A consequence of this is that if you try to use these functions outside of
pldhash.cpp you get a link error. Hmm.
https://hg.mozilla.org/mozilla-central/rev/c840195920bd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1059551
(Assignee)

Updated

4 years ago
Blocks: 1128407
You need to log in before you can comment on or make changes to this bug.