[Openmp-commits] [PATCH] D134396: [OpenMP][libomptarget] New plugin infrastructure and new CUDA plugin

[Kevin Sala Penadés via Phabricator via Openmp-commits]

kevinsala added a comment.

In D134396#3878444 <https://reviews.llvm.org/D134396#3878444>, @JonChesterfield wrote:

> I haven't reviewed this but the following comment stuck out:
>
>> where some threads may be consulting an already inserted ELFObjectFile (without holding the map lock) while another potential thread may be inserting a new one (with the lock acquired).
>
> It's never safe to have one thread reading from a hashtable while another writes to it. Even if the keys were always different there is table resizing and hash collisions to worry about.
>
> Either we could use a persistent structure and CAS the new one in place (bit unusual for this codebase) or a single writer multi reader lock, such that multiple readers can make progress at a time but the whole world grinds to a halt for each writer.

My comment about multiple threads reading the ELF object map was referring to my initial version. In that version, the `std::unordered_map` was protected with a mutex for calling `find` and `insert` operations (could have been improved with a readers-writers lock). Once the reference to a map's element is retrieved, the threads (i.e. mutex no longer acquired) should be able to read safely the element at the same time another thread is inserting to the map (i.e. mutex acquired). However, it seems that the synchronization was not needed since the loading of images (where we actually access the ELF object map) is already serialized, so we removed the lock and switched to `llvm::DenseMap`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134396/new/

https://reviews.llvm.org/D134396