Conversation
|
|
||
| PHP_INSTALL_HEADERS([ext/uuid], m4_normalize([ | ||
| php_uuid.h | ||
| uuidv7-h/php_uuid.h |
There was a problem hiding this comment.
This should be uuidv7.h right?
| uuidv7-h/php_uuid.h | |
| uuidv7-h/uuidv7.h |
|
|
||
| #else | ||
|
|
||
| ts->tv_sec = zend_time_real_get(); |
There was a problem hiding this comment.
| ts->tv_sec = zend_time_real_get(); | |
| ts->tv_sec = zend_time_real_sec(); |
Is this a typo? We don't get a definition of zend_time_real_get yet(?)
|
|
||
| ZEND_ATTRIBUTE_NONNULL_ARGS(1) PHPAPI zend_result php_uuid_v7_parse(const zend_string *uuid_str, php_uuid_v7 uuid) | ||
| { | ||
| int result = uuidv7_from_string(ZSTR_VAL(uuid_str), uuid); |
There was a problem hiding this comment.
since uuidv7_from_string only check if the input is a valid uuid but not a valid uuidv7. I suggest we can have a function like
static zend_always_inline bool php_uuid_v7_is_valid(const php_uuid_v7 uuid)
{
return (uuid[6] & 0xf0) == 0x70 && (uuid[8] & 0xc0) == 0x80;
}To specifically check if the input is uuidv7
There was a problem hiding this comment.
Yes, very nice catch! I think this piece of code should be added to the upstream library in fact.
| case UUIDV7_STATUS_CLOCK_ROLLBACK: | ||
| ZEND_FALLTHROUGH; | ||
| case UUIDV7_STATUS_ERR_TIMESTAMP: | ||
| ZEND_FALLTHROUGH; |
There was a problem hiding this comment.
Would this be clearer?
| ZEND_FALLTHROUGH; | |
| zend_throw_error(NULL, "The generated UUID v7 timestamp is out of range"); | |
| RETURN_THROWS(); |
There was a problem hiding this comment.
I haven't had time to research/think about error handling yet, but it's definitely a good idea.
| case UUIDV7_STATUS_ERR_TIMESTAMP: | ||
| ZEND_FALLTHROUGH; | ||
| case UUIDV7_STATUS_ERR_TIMESTAMP_OVERFLOW: | ||
| break; |
There was a problem hiding this comment.
| break; | |
| zend_throw_error(NULL, "The generated UUID v7 timestamp overflowed"); | |
| RETURN_THROWS(); |
| uuid_object->is_initialized = false; | ||
| } | ||
|
|
||
| PHP_METHOD(Uuid_UuidV7, generate) |
There was a problem hiding this comment.
This uses a monotonic clock when no DateTimeImmutable is passed. UUID v7 is supposed to encode Unix wall-clock time, so on platforms where zend_hrtime() is available this will produce timestamps based on uptime rather than the Unix epoch. So I'd suggest to implement a php_uuid_current_unix_time_ms() here, it could also be used in other time-based uuids like uuidv1
| } | ||
|
|
||
| uint8_t random_bytes[10]; | ||
| for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
The default Random\Engine is MT19937 which is PRNG. In the RFC of uuidv7 the random bytes should be generated in CSPRNG, and therefore I suggest using Random\CryptoSafeEngine instead here.
There was a problem hiding this comment.
therefore I suggest using
Random\CryptoSafeEngineinstead here.
Not necessary to restrict this to CryptSafeEngines, but the default should indeed be Random\Engine\Secure.
There was a problem hiding this comment.
side-node: This should also apply in the future implementation of UUIDv1 and UUIDv6 when generating nodes
as per the RFC and in other places we could just use PRNG to enhance performance.
| object_init_ex(return_value, Z_CE_P(ZEND_THIS)); | ||
| php_uuid_v7_object *uuid_object = Z_UUID_V7_OBJECT_P(return_value); | ||
|
|
||
| if (uuidv7_from_string(ZSTR_VAL(uuid_str), uuid_object->uuid) == FAILURE) { | ||
| zend_throw_exception(NULL, "The specified UUID v7 is malformed", 0); | ||
| RETURN_THROWS(); |
There was a problem hiding this comment.
If I am understanding this code correctly, this could be clearer:
| object_init_ex(return_value, Z_CE_P(ZEND_THIS)); | |
| php_uuid_v7_object *uuid_object = Z_UUID_V7_OBJECT_P(return_value); | |
| if (uuidv7_from_string(ZSTR_VAL(uuid_str), uuid_object->uuid) == FAILURE) { | |
| zend_throw_exception(NULL, "The specified UUID v7 is malformed", 0); | |
| RETURN_THROWS(); | |
| if (php_uuid_v7_parse(uuid_str, uuid) == FAILURE) { | |
| zend_throw_exception(NULL, "The specified UUID is not a valid UUID v7", 0); | |
| RETURN_THROWS(); |
| if (zend_hash_num_elements(Z_ARRVAL_P(arr)) > 0) { | ||
| zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(uuid_object->std.ce->name)); | ||
| RETURN_THROWS(); | ||
| } |
There was a problem hiding this comment.
To make it readonly.
| } | |
| } | |
| uuid_object->is_initialized = true; |
| memcpy(new_uuid_object->uuid, uuid_object->uuid, sizeof(php_uuid_v7)); | ||
| zend_objects_clone_members(&new_uuid_object->std, &uuid_object->std); | ||
|
|
There was a problem hiding this comment.
Mark this as is_initialized after we clone it.
| memcpy(new_uuid_object->uuid, uuid_object->uuid, sizeof(php_uuid_v7)); | |
| zend_objects_clone_members(&new_uuid_object->std, &uuid_object->std); | |
| memcpy(new_uuid_object->uuid, uuid_object->uuid, sizeof(php_uuid_v7)); | |
| new_uuid_object->is_initialized = true; | |
| zend_objects_clone_members(&new_uuid_object->std, &uuid_object->std); | |
|
|
||
| uint64_t unix_time_ms; | ||
| if (datetime_object == NULL) { | ||
| unix_time_ms = zend_time_mono_fallback_nsec() / 1000000; |
There was a problem hiding this comment.
mono is not the correct block to use, it can only be used to measure durations within a single execution.
| } | ||
|
|
||
| uint8_t random_bytes[10]; | ||
| for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
therefore I suggest using
Random\CryptoSafeEngineinstead here.
Not necessary to restrict this to CryptSafeEngines, but the default should indeed be Random\Engine\Secure.
| uint8_t random_bytes[10]; | ||
| for (int i = 0; i < 10; i++) { | ||
| random_bytes[i] = php_random_range(random_algo, 0, 127); | ||
| } |
There was a problem hiding this comment.
why we are random-ing only bytes between 0 and 127🤔 I know its ASCII but not all ASCII bytes are printable, then there is no actual meaning of rander-ing between these bytes. OR maybe I am getting things wrong.
There was a problem hiding this comment.
why we are random-ing only bytes between 0 and 127
Ah no, it was just a typo, I meant to write 0, 255.
This whole loop looks fishy.
uuidv7_generate() expects a const uint8_t *rand_bytes argument, so this was the best thing I could do. I'm happy to receive any advice how it should be done.
There was a problem hiding this comment.
uuidv7_generate()expects aconst uint8_t *rand_bytesargument, so this was the best thing I could do. I'm happy to receive any advice how it should be done.
We should probably expose Randomizer::getBytes() as a PHPAPI for use by extensions. I can look into that once the RFC has moved further.
|
As this is just a POC, I'm not going to work on the implementation much until I finish writing the RFC (thus future review comments may be unaddressed for a while). |
This is a good RFC to me as I too think the UUID extension is required (and actually try to write this RFC months ago but I just don't have enough effort and time on it). Happy to see someone else would like to do this. I have worked on the UUID thing before in the python language so I'd be happy to contribute to your implementation in the future :) Oh and btw is the VCS thing still working now? I would like to join in the voting process in the future but I know we are now using github instead of git.php.net |
|
@LamentXU123 and Tim: Thanks for your reviews, it's pretty much appreciated now and later. I got some insights which I didn't know before :) P.S. I have addressed all the existing comments locally, but I yet to have to push my changes. |
Native support for UUIDs is clearly missing from PHP, so the newly added UUID extension would fill this void. For now, only v7 is implemented, just for demonstration purposes.
I wanted to benchmark how a native implementation compares against
symfony/uidandramsey/uuidperformance-wise. Here are the initial results (with the caveat that I have not verified yet the correctness of the native version, and monotonic ordering is not yet guaranteed):PHP UUID - 50 iterations, 20 warmups, 10 requests (sec)
Symfony UUID - 50 iterations, 20 warmups, 10 requests (sec)
Ramsey UUID - 50 iterations, 20 warmups, 10 requests (sec)