[webkit-reviews] review granted: [Bug 223241] Fix race condition in ConcurrentPtrHashSet. : [Attachment 423350] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 223241: Fix race condition in ConcurrentPtrHashSet.
https://bugs.webkit.org/show_bug.cgi?id=223241

Attachment 423350: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=423350&action=review




--- Comment #3 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 423350
  --> https://bugs.webkit.org/attachment.cgi?id=423350
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=423350&action=review

r=me, looks good!

> Source/WTF/wtf/ConcurrentPtrHashSet.cpp:157
> +    // addSlow() will always start by exchangeAdd'ing 1 to the current
m_table's
> +    // load value before checking if it exceeds its max allowed load. For
the
> +    // real m_table, this is not an issue because at most, it will
accummulate
> +    // up to N extra adds above max load, where N is the number of GC marker
> +    // threads. However, if m_table may be replaced with m_stubTable for
each
> +    // resize operation. As a result, the cummulative error on its load
value
> +    // may far exceed N (as specified above). To fix this, we always reset
it
> +    // here to prevent an overflow. Note: a load of stubDefaultLoadValue
means
> +    // that m_stubTable is full since its size is 0.
> +    //
> +    // In practice, this won't matter because we most likely won't do so
many
> +    // resize operations such that this will get to the point of
overflowing.
> +    // However, since resizing is not in the fast path, let's just be
pedantic
> +    // and reset it for correctness.
> +    m_stubTable.load.store(Table::stubDefaultLoadValue);

Should we reset it after `m_table.store(newTable.get());`?