You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@arrow.apache.org by Gene Novark <ge...@pdtpartners.com.INVALID> on 2019/01/04 22:09:47 UTC
Thread safety of C++ Struct/Schema::GetFieldIndex
These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
Thanks,
Gene
________________________________
This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.
Re: Thread safety of C++ Struct/Schema::GetFieldIndex
Posted by Antoine Pitrou <an...@python.org>.
However, eager initialization could probably work too. I'm not sure why
we used lazy initialization like this. Perhaps someone worried about
the cost of incremental schema construction using repeated
Schema::AddField() calls (but that's gonna be wasteful anyway)?
Le 04/01/2019 à 23:42, Antoine Pitrou a écrit :
>
> In other words, something like:
>
> class StructType {
> mutable std::shared_ptr<std::unordered_map<std::string, int>> name_to_index_;
>
> std::shared_ptr<unordered_map<std::string, int>> GetNameToIndex() const {
> if (!std:atomic_load(&name_to_index_)) {
> name_to_index = std::make_shared<std::unordered_map<std::string, int>>();
> // ... initialize name_to_index ...
> std::atomic_store(&name_to_index_, name_to_index);
> }
> return std:atomic_load(&name_to_index_);
> }
> };
>
>
>
>
> Le 04/01/2019 à 23:32, Antoine Pitrou a écrit :
>>
>> The move-assigning would definitely not be thread-safe.
>>
>> One possibility would be to wrap the std::unordered_map in a
>> std::shared_ptr, and use the atomic functions for shared_ptr:
>> https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
>>
>> Regards
>>
>> Antoine.
>>
>>
>> Le 04/01/2019 à 23:17, Wes McKinney a écrit :
>>> hi Gene,
>>>
>>> Yes, feel free to submit a PR to fix this. I would suggest populating
>>> function-local std::unordered_map and then move-assigning it into
>>> name_to_index_ -- I think this should not have race conditions. If you
>>> do want to add a mutex, it could be a static one rather than creating
>>> a new mutex for each StructType
>>>
>>> Thanks
>>> Wes
>>>
>>> On Fri, Jan 4, 2019 at 4:10 PM Gene Novark <ge...@pdtpartners.com.invalid> wrote:
>>>>
>>>> These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
>>>>
>>>> Thanks,
>>>> Gene
>>>>
>>>>
>>>> ________________________________
>>>> This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
>>>>
>>>> If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
>>>>
>>>> Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.
Re: Thread safety of C++ Struct/Schema::GetFieldIndex
Posted by Antoine Pitrou <an...@python.org>.
In other words, something like:
class StructType {
mutable std::shared_ptr<std::unordered_map<std::string, int>> name_to_index_;
std::shared_ptr<unordered_map<std::string, int>> GetNameToIndex() const {
if (!std:atomic_load(&name_to_index_)) {
name_to_index = std::make_shared<std::unordered_map<std::string, int>>();
// ... initialize name_to_index ...
std::atomic_store(&name_to_index_, name_to_index);
}
return std:atomic_load(&name_to_index_);
}
};
Le 04/01/2019 à 23:32, Antoine Pitrou a écrit :
>
> The move-assigning would definitely not be thread-safe.
>
> One possibility would be to wrap the std::unordered_map in a
> std::shared_ptr, and use the atomic functions for shared_ptr:
> https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
>
> Regards
>
> Antoine.
>
>
> Le 04/01/2019 à 23:17, Wes McKinney a écrit :
>> hi Gene,
>>
>> Yes, feel free to submit a PR to fix this. I would suggest populating
>> function-local std::unordered_map and then move-assigning it into
>> name_to_index_ -- I think this should not have race conditions. If you
>> do want to add a mutex, it could be a static one rather than creating
>> a new mutex for each StructType
>>
>> Thanks
>> Wes
>>
>> On Fri, Jan 4, 2019 at 4:10 PM Gene Novark <ge...@pdtpartners.com.invalid> wrote:
>>>
>>> These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
>>>
>>> Thanks,
>>> Gene
>>>
>>>
>>> ________________________________
>>> This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
>>>
>>> If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
>>>
>>> Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.
Re: Thread safety of C++ Struct/Schema::GetFieldIndex
Posted by Antoine Pitrou <an...@python.org>.
Generally speaking, normal assignments are not thread-safe. Intuitively
they could be (and perhaps in some simple cases - such as aligned
machine types - they will turn up to be on some specific compiler/CPU
combinations), but C++ makes no guarantee about that (for example, an
assignment can be performed in several steps by the CPU, in which case
another thread can see a half-finished assignment). You need to use
either an atomic or some means of synchronizing access (such as a mutex).
Regards
Antoine.
Le 04/01/2019 à 23:42, Wes McKinney a écrit :
> That works. I would have thought that the deterministic state of
> unordered_map might make move-assignment safe, but perhaps not
>
> On Fri, Jan 4, 2019 at 4:33 PM Antoine Pitrou <an...@python.org> wrote:
>>
>>
>> The move-assigning would definitely not be thread-safe.
>>
>> One possibility would be to wrap the std::unordered_map in a
>> std::shared_ptr, and use the atomic functions for shared_ptr:
>> https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
>>
>> Regards
>>
>> Antoine.
>>
>>
>> Le 04/01/2019 à 23:17, Wes McKinney a écrit :
>>> hi Gene,
>>>
>>> Yes, feel free to submit a PR to fix this. I would suggest populating
>>> function-local std::unordered_map and then move-assigning it into
>>> name_to_index_ -- I think this should not have race conditions. If you
>>> do want to add a mutex, it could be a static one rather than creating
>>> a new mutex for each StructType
>>>
>>> Thanks
>>> Wes
>>>
>>> On Fri, Jan 4, 2019 at 4:10 PM Gene Novark <ge...@pdtpartners.com.invalid> wrote:
>>>>
>>>> These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
>>>>
>>>> Thanks,
>>>> Gene
>>>>
>>>>
>>>> ________________________________
>>>> This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
>>>>
>>>> If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
>>>>
>>>> Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.
Re: Thread safety of C++ Struct/Schema::GetFieldIndex
Posted by Wes McKinney <we...@gmail.com>.
That works. I would have thought that the deterministic state of
unordered_map might make move-assignment safe, but perhaps not
On Fri, Jan 4, 2019 at 4:33 PM Antoine Pitrou <an...@python.org> wrote:
>
>
> The move-assigning would definitely not be thread-safe.
>
> One possibility would be to wrap the std::unordered_map in a
> std::shared_ptr, and use the atomic functions for shared_ptr:
> https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
>
> Regards
>
> Antoine.
>
>
> Le 04/01/2019 à 23:17, Wes McKinney a écrit :
> > hi Gene,
> >
> > Yes, feel free to submit a PR to fix this. I would suggest populating
> > function-local std::unordered_map and then move-assigning it into
> > name_to_index_ -- I think this should not have race conditions. If you
> > do want to add a mutex, it could be a static one rather than creating
> > a new mutex for each StructType
> >
> > Thanks
> > Wes
> >
> > On Fri, Jan 4, 2019 at 4:10 PM Gene Novark <ge...@pdtpartners.com.invalid> wrote:
> >>
> >> These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
> >>
> >> Thanks,
> >> Gene
> >>
> >>
> >> ________________________________
> >> This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
> >>
> >> If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
> >>
> >> Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.
Re: Thread safety of C++ Struct/Schema::GetFieldIndex
Posted by Antoine Pitrou <an...@python.org>.
The move-assigning would definitely not be thread-safe.
One possibility would be to wrap the std::unordered_map in a
std::shared_ptr, and use the atomic functions for shared_ptr:
https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
Regards
Antoine.
Le 04/01/2019 à 23:17, Wes McKinney a écrit :
> hi Gene,
>
> Yes, feel free to submit a PR to fix this. I would suggest populating
> function-local std::unordered_map and then move-assigning it into
> name_to_index_ -- I think this should not have race conditions. If you
> do want to add a mutex, it could be a static one rather than creating
> a new mutex for each StructType
>
> Thanks
> Wes
>
> On Fri, Jan 4, 2019 at 4:10 PM Gene Novark <ge...@pdtpartners.com.invalid> wrote:
>>
>> These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
>>
>> Thanks,
>> Gene
>>
>>
>> ________________________________
>> This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
>>
>> If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
>>
>> Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.
Re: Thread safety of C++ Struct/Schema::GetFieldIndex
Posted by Wes McKinney <we...@gmail.com>.
hi Gene,
Yes, feel free to submit a PR to fix this. I would suggest populating
function-local std::unordered_map and then move-assigning it into
name_to_index_ -- I think this should not have race conditions. If you
do want to add a mutex, it could be a static one rather than creating
a new mutex for each StructType
Thanks
Wes
On Fri, Jan 4, 2019 at 4:10 PM Gene Novark <ge...@pdtpartners.com.invalid> wrote:
>
> These are both effectively-immutable accessors with lazy initialization. However, when accessed from multiple threads a race can occur initializing the name_to_index_ map. This seems like a bug rather than a purposeful design choice based off the cpp/conventions.rst section on Immutability. I'm happy to send a PR if this is agreed to be a bug and what an acceptable fix is (e.g. just eagerly initialize, always use a mutex, last-writer-wins with some sort of atomic ptr, etc.).
>
> Thanks,
> Gene
>
>
> ________________________________
> This communication is intended only for the addressee(s), may contain confidential, privileged or proprietary information, and may be protected by US and other laws. Your acceptance of this communication constitutes your agreement to keep confidential all the confidential information contained in this communication, as well as any information derived by you from the confidential information contained in this communication. We do not waive any confidentiality by misdelivery.
>
> If you receive this communication in error, any use, dissemination, printing or copying is strictly prohibited; please destroy all electronic and paper copies and notify the sender immediately. Nothing in this email is intended to constitute (1) investment or trading advice or recommendations or any advertisement or (2) a solicitation of an investment in any jurisdiction in which such a solicitation would be unlawful.
>
> Please note that PDT Partners, LLC, including its affiliates, reserves the right to intercept, archive, monitor and review all communications to and from its network.