1149888 - Make PLDHashTable::mRecursionLevel atomic

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

PLDHashTable::mRecursionLevel should be atomic. I think that there are safe usage patterns on multiple threads where this value can become incorrect (and assertions fail) without an atomic counter, like:

  Thread 1 (e.g. the main thread):
    - Modify table while holding designated lock
      - INCREMENT_RECURSION_LEVEL()
      - DECREMENT_RECURSION_LEVEL()
    - Read from table without holding designated  lock
      - (safe because modifications only happen on Thread 1)
      - INCREMENT_RECURSION_LEVEL()
      - DECREMENT_RECURSION_LEVEL()

  Thread 2 (e.g. some worker thread):
    - Read from table while holding designated lock:
      - INCREMENT_RECURSION_LEVEL()
      - DECREMENT_RECURSION_LEVEL()

Also, it's only used on DEBUG builds, so the perf hit should be acceptable.

I guess the only real issue is that this increases the size of a PLDHashTable by sizeof(uint32_t).

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Though, I wonder if we don't want something more like a per-thread recursion level...

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Once this is fixed I need to back out anything that landed from bug 1149894.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8586582 [details] [diff] [review]
Patch, v1

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

Yuck.

::: xpcom/glue/pldhash.h
@@ +196,5 @@
> +  /*
> +   * |mRecursionLevel| is only used in debug builds, but is present in opt
> +   * builds to avoid binary compatibility problems when mixing DEBUG and
> +   * non-DEBUG components. Make it protected to suppress -Wunused-private-field
> +   * warnings in opt builds.

Now that we've had to move this field anyway, would you please file a follow-up to make this field DEBUG-only?  struct padding should ensure that there's still a 16-bit hole where this once was, it's entirely possible (and completely unwarranted) that binary things make assumptions about the size of PLDHashTable, and we might as well not make hash tables bigger in opt builds in any event.  Thanks.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

This works better!

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69a97ef36fe9 looks happy

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8587712 [details] [diff] [review]
Patch, v2

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

r=me assuming modulo comments below, especially the one on the relaxed mRecursionLevel check.

::: xpcom/glue/pldhash.cpp
@@ +39,5 @@
>   * incremented it).
>   *
>   * Only PL_DHashTableFinish needs to allow this special value.
>   */
> +#define IMMUTABLE_RECURSION_LEVEL ((uint32_t)-1)

Might as well just make this UINT32_MAX?

@@ +49,5 @@
>  #define INCREMENT_RECURSION_LEVEL(table_)                                     \
>      do {                                                                      \
> +        if (table_->mRecursionLevel != IMMUTABLE_RECURSION_LEVEL) {           \
> +            const uint32_t oldRecursionLevel = table_->mRecursionLevel++;     \
> +            MOZ_ASSERT(oldRecursionLevel < UINT32_MAX);                       \

This assert is always going to be true, since mRecursionLevel must be < UINT32_MAX if we're entering this if.  Maybe you meant |< UINT32_MAX - 1|?

Actually, come to think of it, we ought to be using IMMUTABLE_RECURSION_LEVEL in this check, shouldn't we?

@@ +817,5 @@
>        entryAddr -= tableSize;
>      }
>    }
>  
> +  MOZ_ASSERT(!didRemove || mRecursionLevel >= 1);

So you said over IRC that this relaxed check of mRecursionLevel was to enable multiple threads to enumerate a hashtable...but shouldn't that be handled by the !didRemove check?  I guess you could have one designated thread removing things while the other thread(s) were just reading, but that seems like asking for trouble.

It's not great that we can now nest Enumerate() calls inside of other hashtable API calls, but I don't think we want to try and make the recursion level count both enumerate calls and normal API calls...

::: xpcom/glue/pldhash.h
@@ +241,5 @@
> +    mStats = Move(aOther.mStats);
> +#endif
> +
> +#ifdef DEBUG
> +    // Atomic<> doesn't have an |operator=(const Atomic<>&)|.

Did you typo this and mean |operator=(Atomic<>&&)|?

Either way, probably worth filing an MFBT bug for this.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3974ad5ec734

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6)
> Might as well just make this UINT32_MAX?

Ok.

> Maybe you meant |< UINT32_MAX - 1|?

Yep!

> Actually, come to think of it, we ought to be using
> IMMUTABLE_RECURSION_LEVEL in this check, shouldn't we?

Ok.

> So you said over IRC that this relaxed check of mRecursionLevel was to
> enable multiple threads to enumerate a hashtable...but shouldn't that be
> handled by the !didRemove check?

Yeah, I think you're right. I changed this back.

> Did you typo this and mean |operator=(Atomic<>&&)|?

Yep, forgot to update my comment when I switched to using Move.

> Either way, probably worth filing an MFBT bug for this.

I'm not sure about this... Forcing people to think about what they really want to do when dealing with atomics is kind of a nice feature...

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3974ad5ec734

Comment 10

[Nicholas Nethercote [inactive]]

I'm not an expert on move constructors, but I think the new move constructor has some problems:

- It doesn't destroy |this| before copying |aOther|'s bits and pieces across, which means that |this|'s contents are leaked. Calling Finish() on |this| as a first step would avoid this problem.

- It lacks a |this != &aOther| test at the start. Might not matter with the current code but is good hygiene.

- It doesn't do anything to put |aOther| into a safe empty state. Ditto about the hygiene.

Here's an alternative version that I have sitting around in a patch queue that addresses these problems.

> PLDHashTable& PLDHashTable::operator=(PLDHashTable&& aOther)
> {
>   if (this != &aOther) {
>     // Destruct |this|.
>     Finish();
> 
>     // Copy pieces over.
>     mOps = aOther.mOps;
>     mHashShift = aOther.mHashShift;
> #ifdef DEBUG
>     mRecursionLevel = aOther.mRecursionLevel;
> #endif
>     mEntrySize = aOther.mEntrySize;
>     mEntryCount = aOther.mEntryCount;
>     mRemovedCount = aOther.mRemovedCount;
>     mGeneration = aOther.mGeneration;
>     mEntryStore = aOther.mEntryStore;
> #ifdef PL_DHASHMETER
>     mStats = aOther.mStats;
> #endif
> 
>     // Clear up |aOther| so its destruction will be a no-op.
>     aOther.mOps = nullptr;
> #ifdef DEBUG
>     aOther.mRecursionLevel = 0;
> #endif
>     aOther.mEntryStore = nullptr;
>   }
>   return *this;
> }

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> but I think the new move constructor has some problems:

Sounds like I was hasty here :( Let's discuss fixes in a new bug.

Comment 12

[Nicholas Nethercote [inactive]]

> Sounds like I was hasty here :( Let's discuss fixes in a new bug.

Bug 1160436 has a fix.