815467 - Store pldhash's recursionLevel in a better place

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

In debug builds, pldhash stores an additional "recursion level" field.
A comment explains:

  The recursion level is stored in additional space allocated at the end of
  the entry store to avoid changing PLDHashTable, which could cause issues
  when mixing DEBUG and non-DEBUG components.

Tacking it onto the end of the storage is annoying when memory profiling in
debug mode, because often you'll have, say, 4096 bytes of storage, and then
the extra field bumps that up 5000, which jemalloc rounds up to 8192.  It's 
also just an ugly hack.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Store pldhash's recursionLevel in a better place.

(dbaron, please forward this review onto anyone else who may be more
suitable.)

This patch moves the recursion level into the PLDHashTable struct, at the end.
This fixes the wrong-size-when-profiling problem, and still allows
debug/non-debug mixing (even though I suspect it's not really necessary).

BTW, I was surprised to see that pldhash.o is built four times:

./xpcom/glue/standalone/pldhash.o
./xpcom/glue/pldhash.o
./xpcom/glue/nomozalloc/pldhash.o
./xpcom/build/pldhash.o

Yikes!

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 685434 [details] [diff] [review]
Store pldhash's recursionLevel in a better place.

>+#ifdef DEBUG
>+    /*
>+     * |recursionLevel| is stored at the end of this struct to avoid issues
>+     * when mixing DEBUG and non-DEBUG components.  (That assumes PL_DHASHMETER
>+     * isn't also defined;  it rarely is.)
>+     */
>+    uint32_t            recursionLevel; /* used to detect unsafe re-entry */
>+#endif

If you:

 * build a DEBUG build

 * use an extension with a binary component that was built non-DEBUG

 * that extension allocates (on stack or heap) a structure (perhaps an nsTHashtable derivative) that contains a PLDHashTable

then you're still going to get into trouble.  So storing at the end isn't good enough; this comment is false.


The question is if there are any situations like that that we still care about.  Benjamin would probably know that better than I.

Comment 3

[Benjamin Smedberg]

Comment on attachment 685434 [details] [diff] [review]
Store pldhash's recursionLevel in a better place.

I don't think this really improves things much, but see also bug 810718 where we could be using recursionLevel in release builds. Perhaps we should just stop making the member debug-only.

Comment 4

[Nicholas Nethercote [inactive]]

> Perhaps we should
> just stop making the member debug-only.

I'd be fine with that.  It would increase sizeof(PLDHashTable) on 32-bit from 32 bytes to 36, if my calculcations are correct, which doesn't seem like a big deal.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Store pldhash's recursionLevel in a better place.

To recap this bug:

- In debug builds, an extra word is added to the entryStorage for the
  "recursionLevel".  This causes debug build pldhashes to use more memory,
  sometimes a lot, which is annoying.

- I tried moving |recursionLevel| into PLDHashTable for debug builds only.
  dbaron pointed out that this would cause problems when mixing debug and
  non-debug components.

- bsmedberg suggested putting |recursionLevel| in PLDHashTable in all builds to
  avoid the mixed-component problem.

- I gave up and closed the bug.

- (Time passes...)

- We recently started getting lots of Win7 Mochi-2 failures, but only on debug
  builds, due to OOM.  This is because CC sometimes OOMs during shutdown, and
  often the OOMs are occurring in a pldhash.  Although the underlying problems
  are deeper (see bug 932781), it's a reminder that the significant size
  difference of pldhash in debug and non-debug builds is undesirable.

- So I've decided to implement bsmedberg's suggestion.

The attached patch does that.  It's identical to the previous patch, except for
the non-conditional inclusion of |recursionLevel| in PLDHashTable.

This patch does increase sizeof(PLDHashTable) by 4 bytes, but in the recent bug
927705 I reduced sizeof(PLDHashTable) by 8 bytes, so this doesn't worry me.

Comment 6

[Nicholas Nethercote [inactive]]

> This patch does increase sizeof(PLDHashTable) by 4 bytes, but in the recent
> bug
> 927705 I reduced sizeof(PLDHashTable) by 8 bytes, so this doesn't worry me.

Actually, does recursionLevel need to be uint32_t?  I think it will safely fit into uint16_t, in which case we can put it next to |hashShift| and sizeof(PLDHashTable) will be unchanged.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> > This patch does increase sizeof(PLDHashTable) by 4 bytes, but in the recent
> > bug
> > 927705 I reduced sizeof(PLDHashTable) by 8 bytes, so this doesn't worry me.
> 
> Actually, does recursionLevel need to be uint32_t?  I think it will safely
> fit into uint16_t, in which case we can put it next to |hashShift| and
> sizeof(PLDHashTable) will be unchanged.

uint16_t should be fine.  (I suspect even uint8_t would be fine.)

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 824843 [details] [diff] [review]
Store pldhash's recursionLevel in a better place.

> #define DECREMENT_RECURSION_LEVEL(table_)                                     \
>     PR_BEGIN_MACRO                                                            \
>-        if (RECURSION_LEVEL(table_) != IMMUTABLE_RECURSION_LEVEL) {           \
>-            MOZ_ASSERT(RECURSION_LEVEL(table_) > 0);                          \
>-            --RECURSION_LEVEL(table_);                                        \
>+        if (table->recursionLevel != IMMUTABLE_RECURSION_LEVEL) {             \
>+            NS_ASSERTION(table->recursionLevel > 0, "table->recursionLevel > 0"); \
>+            --table->recursionLevel;                                          \

leave this as MOZ_ASSERT.

>+    /*
>+     * This is only used in debug builds, but is present in opt builds to avoid
>+     * problems when mixing DEBUG and non-DEBUG components.
>+     */
>+    uint32_t            recursionLevel; /* used to detect unsafe re-entry */

s/This/recursionLevel/, since otherwise the scope of "This" is ambiguous


Changing recursionLevel to uint16_t and putting it next to hashShift is fine, but if you do, you also need to fix IMMUTABLE_RECURSION_LEVEL

r=dbaron either way (uint16_t or uint32_t)

Comment 9

[Nicholas Nethercote [inactive]]

Thanks for the fast review!

> the recent  bug  927705 I reduced sizeof(PLDHashTable) by 8 bytes

I mis-remembered:  I removed two 8-bit fields, which didn't end up changing sizeof(PLDHashTable), due to padding.  Nonetheless, using uint16_t for recursionLevel leaves sizeof(PLDHashTable) unchanged, which is good.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de06b0e593e6

Comment 11

[[e-mail]]

(In reply to Nicholas Nethercote from comment #0)
> Tacking it onto the end of the storage is annoying when memory profiling in
> debug mode, because often you'll have, say, 4096 bytes of storage, and then
> the extra field bumps that up 5000, which jemalloc rounds up to 8192.  It's 
> also just an ugly hack.

That's a big debug-only field ;-)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de06b0e593e6