Re: [PATCH] hashmap: add API to disable item counting when threaded

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Sep 5, 2017, 17:07 UTC
Message-ID: <77c206f8-43f6-4468-3289-096e37b9fead@jeffhostetler.com>
In-Reply-To: <20170902080535.j7dommmxowpbk2no@sigill.intra.peff.net>

On 9/2/2017 4:05 AM, Jeff King wrote:

Show 23 quoted lines

> On Wed, Aug 30, 2017 at 06:59:22PM +0000, Jeff Hostetler wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> This is to address concerns raised by ThreadSanitizer on the
>> mailing list about threaded unprotected R/W access to map.size with my previous
>> "disallow rehash" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).  See:
>> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/
>>
>> Add API to hashmap to disable item counting and to disable automatic rehashing.
>> Also include APIs to re-enable item counting and automatica rehashing.
> 
> It may be worth summarizing the discussion at that thread here.
> 
> At first glance, it looks like this is woefully inadequate for allowing
> multi-threaded access. But after digging, the issue is that we're really
> trying to accommodate one specific callers which is doing its own
> per-bucket locking, and which needs the internals of the hashmap to be
> truly read-only.
> 
> I suspect the code might be easier to follow if that pattern were pushed
> into its own threaded_hashmap that disabled the size and handled the
> mod-n locking, but I don't insist on that as a blocker to this fix.

Agreed. It would be better and easier to understand to produce a thread-safe version of the hashmap code -- there are other uses of the code that are currently doing their own locking that might benefit, but I'm not looking to gratuitously refactor things.

The other issue was that we are multi-threaded during the lazy-init phase, but the main status-or-whatever code that then uses the hashmap is not. So we only pay for the locking during the multi-threaded usage.

Thanks, Jeff