Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:114523 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 93221 invoked from network); 19 May 2021 09:37:15 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 19 May 2021 09:37:15 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id B19B21804D9 for ; Wed, 19 May 2021 02:46:59 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,HTML_MESSAGE, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.2 X-Spam-Virus: No X-Envelope-From: Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Wed, 19 May 2021 02:46:59 -0700 (PDT) Received: by mail-lf1-f49.google.com with SMTP id j6so15457174lfr.11 for ; Wed, 19 May 2021 02:46:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jwDMAo2+FkzTK9vBmDlTY4wyPyqFudYtUP8yvCDa/qU=; b=AxXG1bDtSbXtZtQOx8cJQy2MpXbP9BMFFyL1IzJ0ltylFY7DX8QeLOnIn1PbtLuEnV kvt05ryOg6aw+VF3KpYLKSIdIYuNoPjxH3VzE3AhVPmlkL/hxCDU/K8wjSKUJ8DmTyLt ATZUWsvfQOIglF3Dq+ePXF6DHrvywMLVd6KcyXGSUQUMMsq6+tTeTEnchnCUIkW2ppaE 6wB9F6ZSHcmhyFNHrX8MncDOor7V6oDHTjHDNXusMe1uW4uncpL4YO5Ej3F4xCv0PDfo geje6bgEfelyyXkeToTHNSfyYZ3FydSD2iGTpdcVz3VE38/UW+vXECy8oIReWHYtpqJN MVUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jwDMAo2+FkzTK9vBmDlTY4wyPyqFudYtUP8yvCDa/qU=; b=Cla7nDYstG/5vNCjW4U9BdLd+Ouxes1PtoV4g37KFfuWwbSKzNk3MsAPaHfs1gYP5R MtS15kayayoWWhuODByWJxfD5InScGMyp/toX/b5rEb8/qNUGNjK03zAdAZH/l0PlV8Z McYNPtqn/bJ+e8nH12ZhP1/+B2GMDr5xo5B5tF7EXoDk+i5k9oixfJ2AcZPW+wcpoNIT vqSavaBOwOVkZUe3k6RujoQTALlg7Nose9ndjMQvCa82VHCb3vwIBok77miq1dF/bKTJ 0OQgTuUa0hh4DeE8UsE0EYBIXSSV7dXFa+DPDQuupMu3lBJ5KC1YDMrtJf39ey0d9Ihx sBoQ== X-Gm-Message-State: AOAM530RHWq731RFN8yVzOSvFZ8Ahh74lpfEyDAHT6eqksdfsOb1Pyj+ UUQrJJPm5cXQuJynnD0eF8wEVVY9bSRs/1WMbQ== X-Google-Smtp-Source: ABdhPJznpvNrx0+3QynsBs/CvgpDexf3m7lCycD23qv90f6KEeElqbG/Jzd9juAhjHA5uYK89jnbdJlmWh1hoGqB+Bo= X-Received: by 2002:ac2:4884:: with SMTP id x4mr7996869lfc.119.1621417613000; Wed, 19 May 2021 02:46:53 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: Date: Wed, 19 May 2021 11:46:41 +0200 Message-ID: To: Go Kudo Cc: PHP internals Content-Type: multipart/alternative; boundary="000000000000db052d05c2abb48f" Subject: Re: [PHP-DEV] [RFC] [Draft] Add RNG extension and deprecate mt_srand() From: guilliam.xavier@gmail.com (Guilliam Xavier) --000000000000db052d05c2abb48f Content-Type: text/plain; charset="UTF-8" On Tue, May 18, 2021 at 6:19 PM Go Kudo wrote: > Hello internals. > > I have created a draft of the RFC. > > https://wiki.php.net/rfc/rng_extension > Hello Go Kudo, First of all, thank you for the (re)work. I think the API looks better, but I still have some remarks/questions (roughly in order of appearance, with some overlap): - How many bytes is `Source::next()` supposed to generate? Is it even intended to be called directly (vs `Randomizer::generateBytes(int $length)`)? - The stubs in "Proposal" should show `__construct()` signatures for the classes implementing `Source`. - About the `MT19937PHP` variant: The PHP manual for `mt_srand()` describes the `MT_RAND_PHP` mode like this: "Uses an *incorrect* Mersenne Twister implementation which was used as the default up till PHP 7.1.0. This mode is available for backward compatibility."; do we really need/want to port it to this new extension? - I think you said in the past that `XorShift128Plus` should be the preferred/default implementation, and `MT19937` is only provided for compatibility/migration? But if so, that's not clear at all in the RFC. - `PlatformProvided` feels like the "odd one out" here: its constructor [assuming same as previous RFC] doesn't take a seed, and it isn't serializable; I understand why, but the main point of the RFC is "RNG reproducibility", which this class cannot achieve. Also, we already have `random_bytes()`/`random_int()`, and you're proposing to change `shuffle()`/`str_shuffle()`/`array_rand()`'s internal RNG from `mt_rand()`-like to `random_bytes()`-like, so what reason would there be to use this class? only `generateFloat()`? but couldn't that be a new global function `random_float()`? All in all, wouldn't it be simpler to drop `PlatformProvided`, and have all `Source` implementations take a seed on construction and be serializable? - Technical, about `Randomizer::generateFloat()`: Is the returned float guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other floats? Also, no `generateFloat(float $min, float $max)` variant like for `generateInt()`? - **About main usage of the API:** To get e.g. $n (say 3) pseudo-random integers between $min (say 0) and $max (say 100) using the "best" implementation (which we have to know/guess is XorShift128+, isn't it?), we must: *first* choose a $seed (`time()`, maybe?), *then* do `$source = new XorShift128Plus($seed);` *and* `$rng = new Randomizer($source);` [even if $source (and $seed) could be inlined, that's still several "steps"], *then* we can finally call `$rng->generateInt($min, $max);` $n times. Is that correct? If so, it may make sense from the implementer side, but from the user side it doesn't look like the most friendly. Also, that lets the possibility of constructing a second Randomizer using the same $source (or `$rng->getSource()`), which could be undesirable for reproducibility, isn't it? Wouldn't it be better to "merge" Randomizer into the interface/classes? Actually I see that's the case in your other project ext/orng ( https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/rnginterface.stub.php and https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/xorshift128plus.stub.php ), why the different approach for ext/rng? Maybe even better (and already suggested), have Randomizer constructor take an *optional* algorithm (string, default XorShift128Plus::class?) and an *optional* seed (int, default time()?) and construct the source internally (and don't expose it)? So we could do just `$rng = new Randomizer();` to rely on the defaults (then call `$rng->generateInt($min, $max);` $n times). - Finally, I think the vote should be split into 3 parts: the new classes, the deprecations (which itself could be split between [mt_]srand and [mt_]rand), and the change of internal RNG for shuffle/str_shuffle/array_rand. Best regards, -- Guilliam Xavier --000000000000db052d05c2abb48f--