Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97624
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: softfail (pb1.pair.com: domain gmail.com does not designate 178.62.210.197 as permitted sender)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Mon, 09 Jan 2017 16:07:50 +0200
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: Kazuo Oishi <kazuo@o-ishi.jp>, internals@lists.php.net
In-Reply-To: <CAGa2bXZgh6sS=9QTatRqZK7+33GmUZzZtTarzA_m7DBCWHwpNw@mail.gmail.com>
References: <CAGa2bXZ9EzWHDTBg_ZHGW4KdjiFK4tpE7aMCVTZD5n+UZ2vEWA@mail.gmail.com>
 <7500b0c6a50baf49beac70ae01e8b50d@koti.fimnet.fi>
 <CAGa2bXaZPBYTaXzNDnA32BCBoZ_FzFUnRNYKFY8b1TXAWSwMyg@mail.gmail.com>
 <87o9zhju4d.fsf@lil.giraffy.jp>
 <CAGa2bXb0hxQ2A=5APHcDkjwwRESOpUf-kUL3AMaBYr7dTkrWZg@mail.gmail.com>
 <CAGa2bXZ3wFd3BXsSK-FVQVPQT5h1J-FPh=4AyU+4Zf52MmKm2g@mail.gmail.com>
 <87k2a5jcx5.fsf@lil.giraffy.jp>
 <CAGa2bXZgh6sS=9QTatRqZK7+33GmUZzZtTarzA_m7DBCWHwpNw@mail.gmail.com>
Message-ID: <d43b1d6ad4b47d793acea5b8caf8860c@gmail.com>
User-Agent: Roundcube Webmail/1.2.3
Subject: Re: [PHP-DEV] Use decent entropy for uniqid($prefix, TRUE)
From: lauri.kentta@gmail.com (=?UTF-8?Q?Lauri_Kentt=C3=A4?=)

On 2017-01-09 08:08, Yasuo Ohgaki wrote:
> Hi Kazuo,
> 
> On Mon, Jan 9, 2017 at 9:27 AM, Kazuo Oishi <kazuo@o-ishi.jp> wrote:
> 
>>>>> [original uniqid() using php_combined_lcg]
>>>>> $ time ./php_uniqid_orig -r 'for($i=0; $i<1000000;$i++)
>> uniqid("",true);'
>>>>> real    0m0.366s
>>>>> user    0m0.350s
>>>>> sys     0m0.010s
>>>>> 
>>>>> [your php_random_bytes_throw version (commit
>>>>> 48f1a17886d874dc90867c669481804de90509e8)]
>>>>> $ time ./php_uniqid_yohgaki -r 'for($i=0; $i<1000000;$i++)
>>>>> uniqid("",true);'
>>>>> real    0m4.509s
>>>>> user    0m0.430s
>>>>> sys     0m4.070s
>>>>> 
>>>>> [Lauri's php_random_int version]
>>>>> $ time ./php_uniqid_lauri -r 'for($i=0; $i<1000000;$i++)
>> uniqid("",true);'
>>>>> real    0m0.664s
>>>>> user    0m0.260s
>>>>> sys     0m0.400s
>>>> 
>>>> Interesting result. AFAIK, I didn't get significant difference
>> when I made
>>>> the patch.
>>>> What is your system? It seems your PRNG is significantly slow.
>> 
>> Core i7-5600U 2.60GHz
>> Linux version 4.8.10, gcc version 4.9.3, gentoo
> 
> Thanks.
> I don't see such difference on my
> Fedora 25 Corei7-4770S
> gcc version 6.3.1 20161221 (Red Hat 6.3.1-1) (GCC)
> 
> I'm curious because I don't see performance issue you have.
> I'll send patch next week or so because I'm interested in how modified
> patch will perform on your system.
> 
>>> The performance will be improved by reducing multiple PRNG calls
>> to 1.
>>> I'll modify patch later, could you test it with your system?
>> 
>> Sure.  But as you said, Lauri's version would be optimal.
> 
> I wrote the same patch right after php_random_int() was implemented
> and didn't find any problem. I think I've posted benchmark result in
> the previous uniqid() discussion thread. So I checked my patch again
> and found it should be
> 
> PHP-master]$ git diff
> diff --git a/ext/standard/uniqid.c b/ext/standard/uniqid.c
> index 22173ae..bbd0e0a 100644
> --- a/ext/standard/uniqid.c
> +++ b/ext/standard/uniqid.c
> @@ -35,7 +35,7 @@
>  #include <sys/time.h>
>  #endif
> 
> -#include "php_lcg.h"
> +#include "php_random.h"
>  #include "uniqid.h"
> 
>  /* {{{ proto string uniqid([string prefix [, bool more_entropy]])
> @@ -78,7 +78,9 @@ PHP_FUNCTION(uniqid)
>          * digits for usecs.
>          */
>         if (more_entropy) {
> -               uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
> usec, php_combined_lcg() * 10);
> +               zend_long rand;
> +               php_random_int(0, 999999999, &rand, 1);
> +               uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
> usec, (double)rand/100000000);
>         } else {
>                 uniqid = strpprintf(0, "%s%08x%05x", prefix, sec,
> usec);
>         }
> 
> Notice that int values are less than a billion which is inside of
> signed 32 bit int range. This version is as fast as php_combined_lcg()
> version on my system. Both versions executes a million uniqid() calls
> about 0.36 sec.
> 
> $ php -r '$s = microtime(TRUE);for($i=0;$i<1000000;$i++) uniqid("",
> TRUE); echo microtime(TRUE) - $s;'
> 
> 0.36102104187012
> 
> So above patch would be the final patch. I don't expect issues but if
> there is performace issue on some systems, we may consider Lauri's
> integer computation version.

I can confirm that the integer version is faster (2,6 seconds vs 3,6 
seconds for 1000000 uniqid calls). Testing on ARM (Raspberry Pi) gives 
similar results. It's not surprising, since converting to float/double 
is not free and formatting a floating-point number is also slower than 
formatting an integer.

Using floating-point numbers has exactly zero benefits, while now at 
least two people have reported that integers are faster. I think it's 
also much easier to read and understand integers, and your bugs tell the 
same tale. So do you have some actual arguments for your version, or is 
this just ”not invented here”?

Also, I must say that I'm neither for nor agains this change in general; 
I'm discussing only the implementation.

-- 
Lauri Kenttä