[RFC] Add Random Extension #7079

zeriyoshi · 2021-05-31T04:42:57Z

Added Random extension and many classes.

RFC is now available.
https://wiki.php.net/rfc/rng_extension

Available Algorithm's

XorShift128+ (Random\NumberGenerator\XorShift128Plus)
MT19937 (Random\NumberGenerator\MT19937)
secure (uses php_random_byte() internally) (Random\NumberGenerator\Secure)

nikic · 2021-05-31T13:32:56Z

Please keep design discussions to the thread on the PHP internals mailing list (https://externals.io/message/114561). Otherwise relevant parties may not be aware of it.

drupol · 2021-05-31T13:33:53Z

Please keep design discussions to the thread on the PHP internals mailing list (https://externals.io/message/114561). Otherwise relevant parties may not be aware of it.

Sorry it's my first time commenting here, I didn't know that details and will comply. Sorry about that.

zeriyoshi · 2021-05-31T13:46:50Z

@nikic @drupol @kamil-tekiela
I'm sorry. I was on the verge of posting to the Internals ML.
I will post to Internals again after I sort out the implementation and RFCs.

I would be happy to have future design discussions in the following threads.

https://externals.io/message/114561

TysonAndre

EXTENSION("random" should create module symbols with random, not php_random (excluding static methods or C functions exposed to other modules in the header)

(both by convention and to avoid build issues - I forget exactly where it's important)

TysonAndre · 2021-06-26T13:47:39Z

ext/random/php_random_module.h

@@ -0,0 +1,67 @@
+/*


The file name is wrong and will cause build issues, I think. This should be named php_random.h instead. See build/print_include.awk

../php-src/ext/tokenizer/php_tokenizer.h 21:#define phpext_tokenizer_ptr &tokenizer_module_entry ../php-src/ext/xsl/php_xsl.h 21:#define phpext_xsl_ptr &xsl_module_entry

If changed to php_random.h, the include guard conflicts with php_random.h in ext/standard.
Use PHP_RANDOM_H_MODULE for the include guard only.

TysonAndre · 2021-06-26T13:48:01Z

ext/random/php_random_module.h

+#ifndef PHP_RANDOM_MODULE_H
+#define PHP_RANDOM_MODULE_H
+
+extern zend_module_entry php_random_module_entry;


nit: I don't remember if this affects the build (it might or might not), but the conventional name would be random_module_entry here and below.

(E.g. if php looks for a symbol with that name when loading a shared library - it very likely might)

TysonAndre · 2021-06-26T13:49:33Z

ext/random/php_random_module.h

+extern zend_module_entry php_random_module_entry;
+#define phpext_random_ptr &php_random_module_entry
+
+extern PHPAPI zend_class_entry *php_random_ce_Random_NumberGenerator_RandomNumberGenerator;


nit: I think random_ce_ would be shorter and follow conventions, e.g. spl starts with spl.

The other names in this header are fine.

extern PHPAPI zend_class_entry *spl_ce_SplFixedArray;

TysonAndre · 2021-06-26T13:55:36Z

ext/random/random.c

+/* }}} */
+
+/* {{{ php_random_module_entry */
+zend_module_entry php_random_module_entry = {


same here for random_module_entry, both in PHP_MINIT(random) and PHP_MINIT_FUNCTION(random)

TysonAndre · 2021-06-26T14:07:43Z

ext/random/random.c

+}
+
+static inline zend_object *php_random_rng_common_init(zend_class_entry *ce, const php_random_rng_algo *algo, const zend_object_handlers *handlers) {
+	php_random_rng *rng = zend_object_alloc(sizeof(php_random_rng), ce);


This seems to be correct - zend_object_alloc is zeroing out state (i.e. setting it to NULL)

/* Allocates object type and zeros it, but not the standard zend_object and properties. * Standard object MUST be initialized using zend_object_std_init(). * Properties MUST be initialized using object_properties_init(). */ static zend_always_inline void *zend_object_alloc(size_t obj_size, zend_class_entry *ce) { void *obj = emalloc(obj_size + zend_object_properties_size(ce)); memset(obj, 0, obj_size - sizeof(zend_object)); return obj; }

Yes, but I'm still getting unintended Valgrind warnings.
Also, the ZTS build on Windows seems to be SEGV on during testing. I do not have a Windows environment, but will continue to try to resolve this.

TysonAndre · 2021-06-26T14:09:41Z

ext/random/random.c

+	php_random_rng *rng = php_random_rng_from_obj(object);
+
+	if (rng->state) {
+		efree(rng->state);


Could set rng->state to NULL just in case - I think the object can be referenced even after __destruct is called.

I don't think that's the issue, though

The object can't be referenced after free_obj, only dtor_obj.

OK, I'll leave this as is.

TysonAndre · 2021-06-26T14:16:21Z

ext/random/random.c

+	ZEND_PARSE_PARAMETERS_END();
+
+	if (!rng) {
+		rng = php_random_rng_xorshfit128plus_new(php_random_ce_Random_NumberGenerator_XorShift128Plus);


Suggested change

rng = php_random_rng_xorshfit128plus_new(php_random_ce_Random_NumberGenerator_XorShift128Plus);

rng = php_random_rng_xorshift128plus_new(php_random_ce_Random_NumberGenerator_XorShift128Plus);

TysonAndre · 2021-06-26T14:27:33Z

ext/random/random.c

+		return php_random_rng_next(random->rng, truncate);
+	}
+
+	rng = Z_OBJ_P(zend_read_property(random->std.ce, &random->std, "rng", sizeof("rng") - 1, 0, NULL));


Should add a check that the zval from zend_read_property is IS_OBJECT.

I'm not sure if using a property is efficient, though - e.g. adding a zend_object *obj to php_random instead would make that a simple pointer lookup.

Also, with Closure->bindTo, it's possible for userland code to call unset($this->std), I think?

For the alternative, it's possible to override get_properties, get_gc, etc. handlers. Look at SplDoublyLinkedList as an example in ext/spl/spl_dll.c

Thank you. I'll take a look.

TysonAndre · 2021-06-26T14:34:52Z

ext/random/random.c

+	return ret;
+}
+
+uint64_t php_random_next(php_random *random, bool truncate)


Suggested change

uint64_t php_random_next(php_random *random, bool truncate)

PHPAPI uint64_t php_random_next(php_random *random, bool truncate)

ext/random/random.stub.php

nikic

(just scrolling through)

ext/random/random.c

nikic · 2021-06-29T14:06:07Z

ext/random/random.c

+	zend_string *rng_fname;
+	zend_function *rng_func;
+
+	if (random->rng) {


I think that userland should also be handled via php_random_rng_algo. It should be created on construction, and the state can cache the zend_function for generate for example. This will make it more efficient, and also make your implementation uniform, because you don't need to distinguish between the userland/internal cases. This is a benefit of the Random class vs the previous free functions, as you can do work during construction now.

Now that Random is a final class, this is a very reasonable opinion.
Thanks. I will implement.

nikic · 2021-06-29T14:07:35Z

ext/random/random.c

+	php_random_rng *rng = php_random_rng_from_obj(object);
+
+	if (rng->state) {
+		efree(rng->state);


The object can't be referenced after free_obj, only dtor_obj.

ext/random/random.c

zeriyoshi · 2021-09-02T15:02:35Z

Create an updated RFC and a new Pull-Request. Thanks.

krakjoe added the RFC label May 31, 2021

zeriyoshi force-pushed the random_class branch 2 times, most recently from 85f6c30 to 01fc380 Compare May 31, 2021 11:04

This comment has been minimized.

Sign in to view

zeriyoshi force-pushed the random_class branch from bf3b284 to dde65a0 Compare June 1, 2021 13:14

Random Extension: First-implementation of RFC 2.0

14de122

zeriyoshi force-pushed the random_class branch from df233ee to 14de122 Compare June 26, 2021 00:35

zeriyoshi changed the title ~~[RFC] Add Random class~~ [RFC] Add Random Extension Jun 26, 2021

Random Extension: fix extension name

e2497f4

TysonAndre reviewed Jun 26, 2021

View reviewed changes

nikic reviewed Jun 29, 2021

View reviewed changes

ext/random/random.stub.php Outdated Show resolved Hide resolved

nikic reviewed Jun 29, 2021

View reviewed changes

zeriyoshi added 8 commits July 1, 2021 13:00

Random Extension: fix some review points

406225d

Random Extension: Drop RandomInterface

8060452

Random Extension: Drop tentative-return-type

e5d9e7f

Random Extension: Combine initialize and generate

cbd47ee

Random Extension: some changes

ee845fe

Random Extension: fix header

c423e79

Random Extension: drop bit-shift

1fb7658

Random Extension: rename extension name

0fcefe3

zeriyoshi and others added 4 commits July 2, 2021 16:39

Merge remote-tracking branch 'upstream/master' into random_class

1d0af1d

Fix uninit value

59b5bbe

Random Extension: Drop Random::nextInt()

3ccc236

Random Extension: minor fixes

af5b15f

zeriyoshi force-pushed the random_class branch from 0eab6c0 to af5b15f Compare July 7, 2021 14:20

Merge remote-tracking branch 'upstream/master' into random_class

f382490

nikic added this to the PHP 8.2 milestone Jul 19, 2021

zeriyoshi closed this Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add Random Extension #7079

[RFC] Add Random Extension #7079

zeriyoshi commented May 31, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nikic commented May 31, 2021

drupol commented May 31, 2021

zeriyoshi commented May 31, 2021

TysonAndre left a comment

TysonAndre Jun 26, 2021

zeriyoshi Jun 27, 2021 •

edited

Loading

TysonAndre Jun 26, 2021

TysonAndre Jun 26, 2021

TysonAndre Jun 26, 2021

TysonAndre Jun 26, 2021

zeriyoshi Jul 1, 2021 •

edited

Loading

TysonAndre Jun 26, 2021

nikic Jun 29, 2021

zeriyoshi Jul 1, 2021

TysonAndre Jun 26, 2021

zeriyoshi Jul 1, 2021

TysonAndre Jun 26, 2021

zeriyoshi Jul 1, 2021

TysonAndre Jun 26, 2021

zeriyoshi Jul 1, 2021

nikic left a comment

nikic Jun 29, 2021

zeriyoshi Jul 1, 2021

nikic Jun 29, 2021

zeriyoshi commented Sep 2, 2021

	rng = php_random_rng_xorshfit128plus_new(php_random_ce_Random_NumberGenerator_XorShift128Plus);
	rng = php_random_rng_xorshift128plus_new(php_random_ce_Random_NumberGenerator_XorShift128Plus);

	uint64_t php_random_next(php_random *random, bool truncate)
	PHPAPI uint64_t php_random_next(php_random *random, bool truncate)

[RFC] Add Random Extension #7079

[RFC] Add Random Extension #7079

Conversation

zeriyoshi commented May 31, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nikic commented May 31, 2021

drupol commented May 31, 2021

zeriyoshi commented May 31, 2021

TysonAndre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeriyoshi Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeriyoshi Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeriyoshi commented Sep 2, 2021

zeriyoshi commented May 31, 2021 •

edited

Loading

zeriyoshi Jun 27, 2021 •

edited

Loading

zeriyoshi Jul 1, 2021 •

edited

Loading