diff mbox series

[v2,2/2] Add single-threaded fast path to rand()

Message ID PAWPR08MB8982DC44B324DE1952A3EE97838EA@PAWPR08MB8982.eurprd08.prod.outlook.com
State Superseded
Headers
Series [v2,1/2] Add random benchmark |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed

Commit Message

Wilco Dijkstra Dec. 12, 2023, 5:43 p.m. UTC 
  Improve performance of rand() and __random() by adding a single-threaded fast
path.  Bench-random-lock shows about 5x speedup on Neoverse V1.

---

Comments

Paul Eggert Dec. 12, 2023, 8:47 p.m. UTC | #1 
On 12/12/23 09:43, Wilco Dijkstra wrote:
> +#include <single-thread.h>
>   #include <limits.h>
>   #include <stddef.h>
>   #include <stdlib.h>
> @@ -288,6 +289,12 @@ __random (void)
>   {
>     int32_t retval;
>   
> +  if (SINGLE_THREAD_P)

Since random.c is sort-of shared with gnulib, would there be any harm in 
using the user-visible "#include <sys/single_threaded.h>" and 
__libc_single_threaded instead? That would lessen the differences 
between the two source files. (I don't know why glibc code sometimes 
includes <sys/single_threaded.h> and sometimes <single-thread.h>.)
Florian Weimer Dec. 13, 2023, 9:09 a.m. UTC | #2 
* Paul Eggert:

> On 12/12/23 09:43, Wilco Dijkstra wrote:
>> +#include <single-thread.h>
>>   #include <limits.h>
>>   #include <stddef.h>
>>   #include <stdlib.h>
>> @@ -288,6 +289,12 @@ __random (void)
>>   {
>>     int32_t retval;
>>   +  if (SINGLE_THREAD_P)
>
> Since random.c is sort-of shared with gnulib, would there be any harm
> in using the user-visible "#include <sys/single_threaded.h>" and
> __libc_single_threaded instead? That would lessen the differences
> between the two source files. (I don't know why glibc code sometimes
> includes <sys/single_threaded.h> and sometimes <single-thread.h>.)

We can put the whole optimization into an #ifdef _LIBC block.
SINGLE_THREAD_P is more optimized than the global __libc_single_threaded
variable, and the difference is likely going to be visible on some
architectures on some architectures.

Thanks,
Florian
Wilco Dijkstra Dec. 13, 2023, 2 p.m. UTC | #3 
Hi,

>> Since random.c is sort-of shared with gnulib, would there be any harm
>> in using the user-visible "#include <sys/single_threaded.h>" and
>> __libc_single_threaded instead? That would lessen the differences
>> between the two source files. (I don't know why glibc code sometimes
>> includes <sys/single_threaded.h> and sometimes <single-thread.h>.)

Which header should be used is confusing indeed. It seems that
sys/single_threaded.h is more frequently used, so I'll change it to that.

> We can put the whole optimization into an #ifdef _LIBC block.
> SINGLE_THREAD_P is more optimized than the global __libc_single_threaded
> variable, and the difference is likely going to be visible on some
> architectures on some architectures.

Given there are many uses of SINGLE_THREAD_P scattered around GLIBC
this should really be done in the header or alternatively gnulib could define
its own SINGLE_THREAD_P (no idea how it works today given that stdio and
malloc have many uses).

Cheers,
Wilco
Carlos O'Donell Dec. 13, 2023, 5:48 p.m. UTC | #4 
On 12/12/23 15:47, Paul Eggert wrote:
> Since random.c is sort-of shared with gnulib, would there be any harm
> in using the user-visible "#include <sys/single_threaded.h>" and
> __libc_single_threaded instead? That would lessen the differences
> between the two source files. (I don't know why glibc code sometimes
> includes <sys/single_threaded.h> and sometimes <single-thread.h>.)
 
Paul,

Please update glibc/SHARED-FILES if random.c is being used by gnulib.

We actively use SHARED-FILES to ensure that gnulib code changes have copyright
assignment to the FSF.

This is explained to new contributors via:
https://sourceware.org/glibc/wiki/Contribution%20checklist#Copyright_FSF_or_disclaimer

Please also note that it is not clear to me that all of the copyright in random.c
is assigned to the FSF given the origins of that code.
diff mbox series

Patch

diff --git a/stdlib/random.c b/stdlib/random.c
index 62f22fac8d58c7977f09c134bf80a797750da645..a22de60a0f96031c74dd5a949b6717c2b0fc321a 100644
--- a/stdlib/random.c
+++ b/stdlib/random.c
@@ -51,6 +51,7 @@ 
    SUCH DAMAGE.*/
 
 #include <libc-lock.h>
+#include <single-thread.h>
 #include <limits.h>
 #include <stddef.h>
 #include <stdlib.h>
@@ -288,6 +289,12 @@  __random (void)
 {
   int32_t retval;
 
+  if (SINGLE_THREAD_P)
+    {
+      (void) __random_r (&unsafe_state, &retval);
+      return retval;
+    }
+
   __libc_lock_lock (lock);
 
   (void) __random_r (&unsafe_state, &retval);