You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "FluorineDog (via GitHub)" <gi...@apache.org> on 2023/03/01 11:49:33 UTC

[GitHub] [datasketches-cpp] FluorineDog opened a new pull request, #347: make random utils thread safe

FluorineDog opened a new pull request, #347:
URL: https://github.com/apache/datasketches-cpp/pull/347

   when using kll_sketch in parallel, it will use the same random engine, make TSAN complain:
   ```
   WARNING: ThreadSanitizer: data race (pid=1760545)
     Read of size 8 at 0x000032e79000 by thread T1097:
       #0 std::__1::mersenne_twister_engine<unsigned long, 32ul, 624ul, 397ul, 31ul, 2567483615ul, 11ul, 4294967295ul, 7ul, 2636928640ul, 15ul, 4022730752ul, 18ul, 1812433253ul>::operator()() ~/ByConity/contrib/libcxx/include/random:2394:25 (clickhouse+0x1eb2bdb9) (BuildId: 76fc9d59c0dfa515)
       #1 std::__1::independent_bits_engine<std::__1::mersenne_twister_engine<unsigned long, 32ul, 624ul, 397ul, 31ul, 2567483615ul, 11ul, 4294967295ul, 7ul, 2636928640ul, 15ul, 4022730752ul, 18ul, 1812433253ul>, 1ul, unsigned int>::__eval(std::__1::integral_constant<bool, true>) ~/ByConity/contrib/libcxx/include/random:3192:19 (clickhouse+0x24db3e8c) (BuildId: 76fc9d59c0dfa515)
       #2 std::__1::independent_bits_engine<std::__1::mersenne_twister_engine<unsigned long, 32ul, 624ul, 397ul, 31ul, 2567483615ul, 11ul, 4294967295ul, 7ul, 2636928640ul, 15ul, 4022730752ul, 18ul, 1812433253ul>, 1ul, unsigned int>::operator()() ~/ByConity/contrib/libcxx/include/random:3112:38 (clickhouse+0x24db3e05) (BuildId: 76fc9d59c0dfa515)
       #3 void datasketches::kll_helper::randomly_halve_up<unsigned long>(unsigned long*, unsigned int, unsigned int) ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_helper_impl.hpp:118:27 (clickhouse+0x24dc73d0) (BuildId: 76fc9d59c0dfa515)
       #4 datasketches::kll_sketch<unsigned long, std::__1::less<unsigned long>, datasketches::serde<unsigned long, void>, std::__1::allocator<unsigned long>>::compress_while_updating() ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_sketch_impl.hpp:728:5 (clickhouse+0x24dc6659) (BuildId: 76fc9d59c0dfa515)
       #5 datasketches::kll_sketch<unsigned long, std::__1::less<unsigned long>, datasketches::serde<unsigned long, void>, std::__1::allocator<unsigned long>>::internal_update() ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_sketch_impl.hpp:200:24 (clickhouse+0x24dc6353) (BuildId: 76fc9d59c0dfa515)
       #6 void datasketches::kll_sketch<unsigned long, std::__1::less<unsigned long>, datasketches::serde<unsigned long, void>, std::__1::allocator<unsigned long>>::update<unsigned long&>(unsigned long&) ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_sketch_impl.hpp:183:26 (clickhouse+0x24dc6011) (BuildId: 76fc9d59c0dfa515)
       #7 DB::Statistics::StatsKllSketchImpl<unsigned long>::update(unsigned long) ~/ByConity/src/Statistics/StatsKllSketchImpl.h:134:18 (clickhouse+0x24dc5f66) (BuildId: 76fc9d59c0dfa515)
    
     Previous write of size 8 at 0x000032e79000 by thread T1127:
       #0 std::__1::mersenne_twister_engine<unsigned long, 32ul, 624ul, 397ul, 31ul, 2567483615ul, 11ul, 4294967295ul, 7ul, 2636928640ul, 15ul, 4022730752ul, 18ul, 1812433253ul>::operator()() ~/ByConity/contrib/libcxx/include/random:2401:10 (clickhouse+0x1eb2bfc1) (BuildId: 76fc9d59c0dfa515)
       #1 std::__1::independent_bits_engine<std::__1::mersenne_twister_engine<unsigned long, 32ul, 624ul, 397ul, 31ul, 2567483615ul, 11ul, 4294967295ul, 7ul, 2636928640ul, 15ul, 4022730752ul, 18ul, 1812433253ul>, 1ul, unsigned int>::__eval(std::__1::integral_constant<bool, true>) ~/ByConity/contrib/libcxx/include/random:3192:19 (clickhouse+0x24db3e8c) (BuildId: 76fc9d59c0dfa515)
       #2 std::__1::independent_bits_engine<std::__1::mersenne_twister_engine<unsigned long, 32ul, 624ul, 397ul, 31ul, 2567483615ul, 11ul, 4294967295ul, 7ul, 2636928640ul, 15ul, 4022730752ul, 18ul, 1812433253ul>, 1ul, unsigned int>::operator()() ~/ByConity/contrib/libcxx/include/random:3112:38 (clickhouse+0x24db3e05) (BuildId: 76fc9d59c0dfa515)
       #3 void datasketches::kll_helper::randomly_halve_down<unsigned long>(unsigned long*, unsigned int, unsigned int) ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_helper_impl.hpp:102:27 (clickhouse+0x24dc7590) (BuildId: 76fc9d59c0dfa515)
       #4 datasketches::kll_sketch<unsigned long, std::__1::less<unsigned long>, datasketches::serde<unsigned long, void>, std::__1::allocator<unsigned long>>::compress_while_updating() ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_sketch_impl.hpp:730:5 (clickhouse+0x24dc6683) (BuildId: 76fc9d59c0dfa515)
       #5 datasketches::kll_sketch<unsigned long, std::__1::less<unsigned long>, datasketches::serde<unsigned long, void>, std::__1::allocator<unsigned long>>::internal_update() ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_sketch_impl.hpp:200:24 (clickhouse+0x24dc6353) (BuildId: 76fc9d59c0dfa515)
       #6 void datasketches::kll_sketch<unsigned long, std::__1::less<unsigned long>, datasketches::serde<unsigned long, void>, std::__1::allocator<unsigned long>>::update<unsigned long&>(unsigned long&) ~/ByConity/tsan/datasketches-prefix/include/DataSketches/kll_sketch_impl.hpp:183:26 (clickhouse+0x24dc6011) (BuildId: 76fc9d59c0dfa515)
       #7 DB::Statistics::StatsKllSketchImpl<unsigned long>::update(unsigned long) ~/ByConity/src/Statistics/StatsKllSketchImpl.h:134:18 (clickhouse+0x24dc5f66) (BuildId: 76fc9d59c0dfa515)
   ```
   
   This patch make the random_utils thread-safe. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] FluorineDog commented on a diff in pull request #347: make random utils thread safe

Posted by "FluorineDog (via GitHub)" <gi...@apache.org>.
FluorineDog commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1133830779


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>
+    random_bit_impl(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count() 
+      + std::hash<std::thread::id>{}(std::this_thread::get_id())));
+  return random_bit_impl();
+}
 
 // common random declarations
 namespace random_utils {
   static std::random_device rd; // possibly unsafe in MinGW with GCC < 9.2
-  static std::mt19937_64 rand(rd());
-  static std::uniform_real_distribution<> next_double(0.0, 1.0);
+  static thread_local std::mt19937_64 rand(rd());

Review Comment:
   It has simliar problem, used by other functions. To make the whole project thread-safe, i think they are necessary. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] FluorineDog commented on a diff in pull request #347: make random utils thread safe

Posted by "FluorineDog (via GitHub)" <gi...@apache.org>.
FluorineDog commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1133834978


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>

Review Comment:
   My consideration to use function as a wrapper, is that by using a wrapper, the whole binary can share an instance per thread, instead of "global static(meaning anonymouse in C++)" will create an instance per (thread x compilation unit), which may be too much. But this is not a big issue, I will change to follow your advice. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] FluorineDog commented on a diff in pull request #347: make random utils thread safe

Posted by "FluorineDog (via GitHub)" <gi...@apache.org>.
FluorineDog commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1133834978


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>

Review Comment:
   My consideration to use function as a wrapper, is that by using a wrapper, the whole binary can share an instance per thread, instead of "global static(meaning anonymous in C++)" will create an instance per (thread x compilation unit, i.e., each cpp file), which may be too much. But this is not a big issue, I will change to follow your advice. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] FluorineDog commented on a diff in pull request #347: make random utils thread safe

Posted by "FluorineDog (via GitHub)" <gi...@apache.org>.
FluorineDog commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1133836563


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence

Review Comment:
   will delete it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] FluorineDog commented on pull request #347: make random utils thread safe

Posted by "FluorineDog (via GitHub)" <gi...@apache.org>.
FluorineDog commented on PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#issuecomment-1466043684

   Sorry to be missing for a week, too busy to open github.com Code is updated,


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] FluorineDog commented on a diff in pull request #347: make random utils thread safe

Posted by "FluorineDog (via GitHub)" <gi...@apache.org>.
FluorineDog commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1133834978


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>

Review Comment:
   My consideration to use function as a wrapper, is that by using a wrapper, the whole binary can share an instance per thread, instead of "global static(meaning anonymous in C++)" will create an instance per (thread x compilation unit), which may be too much. But this is not a big issue, I will change to follow your advice. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] coveralls commented on pull request #347: make random utils thread safe

Posted by "coveralls (via GitHub)" <gi...@apache.org>.
coveralls commented on PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#issuecomment-1466712062

   ## Pull Request Test Coverage Report for [Build 4404785640](https://coveralls.io/builds/57805794)
   
   * **0** of **0**   changed or added relevant lines in **0** files are covered.
   * No unchanged relevant lines lost coverage.
   * Overall coverage remained the same at **97.125%**
   
   ---
   
   
   
   |  Totals | [![Coverage Status](https://coveralls.io/builds/57805794/badge)](https://coveralls.io/builds/57805794) |
   | :-- | --: |
   | Change from base [Build 4369952580](https://coveralls.io/builds/57675397): |  0.0% |
   | Covered Lines: | 4898 |
   | Relevant Lines: | 5043 |
   
   ---
   ##### 💛  - [Coveralls](https://coveralls.io)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #347: make random utils thread safe

Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1124992764


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence

Review Comment:
   The comment describes the change, so I don't think it is needed in the code.



##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>

Review Comment:
   Why was this converted to a wrapper? I think this should be simpler like so:
   static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>
     random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()
         + std::hash<std::thread::id>()(std::this_thread::get_id())));



##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>
+    random_bit_impl(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count() 
+      + std::hash<std::thread::id>{}(std::this_thread::get_id())));
+  return random_bit_impl();
+}
 
 // common random declarations
 namespace random_utils {
   static std::random_device rd; // possibly unsafe in MinGW with GCC < 9.2
-  static std::mt19937_64 rand(rd());
-  static std::uniform_real_distribution<> next_double(0.0, 1.0);
+  static thread_local std::mt19937_64 rand(rd());

Review Comment:
   Why did you touch these? They are not used by KLL.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #347: make random utils thread safe

Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1124994698


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>

Review Comment:
   Why was this converted to a wrapper? I think this should be simpler like so:
   static thread_local std::independent_bits_engine&lt;std::mt19937, 1, uint32_t&gt;
     random_bit(static_cast&lt;uint32_t&gt;(std::chrono::system_clock::now().time_since_epoch().count()
         + std::hash&lt;std::thread::id&gt;()(std::this_thread::get_id())));



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #347: make random utils thread safe

Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347#discussion_r1124994698


##########
common/include/common_defs.hpp:
##########
@@ -36,15 +37,22 @@ enum resize_factor { X1 = 0, X2, X4, X8 };
 template<typename A> using AllocChar = typename std::allocator_traits<A>::template rebind_alloc<char>;
 template<typename A> using string = std::basic_string<char, std::char_traits<char>, AllocChar<A>>;
 
-// random bit
-static std::independent_bits_engine<std::mt19937, 1, uint32_t>
-  random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()));
+// thread-safe random bit
+inline uint32_t random_bit() 
+{
+  // add additional hash(this_thread::get_id) 
+  // to make different threads won't share the same random sequence
+  static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>

Review Comment:
   Why was this converted to a wrapper? I think this should be simpler like so:
   ```
   static thread_local std::independent_bits_engine<std::mt19937, 1, uint32_t>
     random_bit(static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count()
         + std::hash<std::thread::id>()(std::this_thread::get_id())));
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org

[GitHub] [datasketches-cpp] AlexanderSaydakov merged pull request #347: make random utils thread safe

Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov merged PR #347:
URL: https://github.com/apache/datasketches-cpp/pull/347


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org