Re: tsan: t3008: hashmap_add touches size from multiple threads

From: Stefan Beller <sbeller@google.com>
Date: Aug 15, 2017, 18:48 UTC
Message-ID: <CAGZ79kb-1S9F4Pp0dzkDX488uiZ8Zu_1m2U=hQ1CcsgSu314rQ@mail.gmail.com>
In-Reply-To: <CAN0heSoXysu=6E_ScfWQVLOk805V=j7AYJi=z62SmNkP5U=A9Q@mail.gmail.com>

Show 14 quoted lines

>>>         /* total number of entries (0 means the hashmap is empty) */
>>> -       unsigned int size;
>>> +       /* -1 means size is unknown for threading reasons */
>>> +       int size;
>>
>> This double-encodes the state of disallow_rehash (i.e. if we had
>> signed size, then the invariant disallow_rehash === (size < 0)
>> is true, such that we could omit either the flag and just check for
>> size < 0 or we do not need the negative size as any user would
>> need to check disallow_rehash first. Not sure which API is harder
>> to misuse. I'd think just having the size and getting rid of
>> disallow_rehash might be hard to to reused.
>
> (Do you mean "might be hard to be misused"?)

yes, I do.

> One good thing about turning off the size-tracking with threading is
> that someone who later wants to know the size in a threaded application
> will not introduce any subtle bugs by misusing size, but will be forced
> to provide and use some sort of InterlockedIncrement().

agreed.

Show 5 quoted lines

> When/if that
> change happens, it would be nice if no-one relied on the value of size
> to say anything about threading. So it might make sense to have an
> implementation-independent way of accessing disallow_rehash a.k.a.
> (size < 0).

Yes, and my point was whether we want to keep disallow_rehash around, as when a patch as this is applied, we'd have it encoded twice, both size < 0 as well as disallow_rehash set indicate the rehashing disabled.

If we were to reduce it to one, we would not have "invalid" state possible such as size < 0 and disallow_rehash = 0.

In the future we may have more options that make size impossible to compute efficiently, such that in that case we'd want to know which condition lead to it. In that case we'd want to have the flags around.

> For example a function hashmap_disallow_rehash(), except that's
> obviously taken. :-) Maybe the existing function would then be
> hashmap_set_disallow_rehash(). Oh well..

Not sure I understand this one.