Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix uniqid() performances #18232

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+15 −6
Open

Fix uniqid() performances #18232

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7cdadcd
On some systems, uniqid() is critically slow.
manu0401 Apr 3, 2025
1877950
Use #ifdef HAVE_UUIDGEN and not #ifdef __NetBSD__ I once used
manu0401 Apr 3, 2025
8e15cf4
Remove an early test that I should have removed earlier.
manu0401 Apr 3, 2025
81adbda
Also remove now unused variable
manu0401 Apr 3, 2025
eb40c48
Avoid using the lower bytes of UUID since it may contain the host MAC
manu0401 Apr 3, 2025
5471a5a
Another attempt to addresse the performance when the system clock
manu0401 Apr 6, 2025
f03c2bc
< instead of <= qo that second member can be used
manu0401 Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ AC_CHECK_FUNCS(m4_normalize([
unsetenv
usleep
utime
uuidgen
vasprintf
]))

Expand Down
54 changes: 53 additions & 1 deletion ext/standard/uniqid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,44 @@
#include <sys/time.h>
#endif

#ifdef HAVE_UUIDGEN
#include <sys/types.h>
#include <sys/uuid.h>
#endif

#include "ext/random/php_random.h"
#include "ext/random/php_random_csprng.h"

#ifdef HAVE_GETTIMEOFDAY
#ifdef HAVE_UUIDGEN
/* {{{ Generates a unique ID */
PHP_FUNCTION(uniqid)
{
char *prefix = "";
bool more_entropy = 0;
zend_string *uniqid;
size_t prefix_len = 0;
struct uuid uuid;
int *n = (int *)&uuid;

ZEND_PARSE_PARAMETERS_START(0, 2)
Z_PARAM_OPTIONAL
Z_PARAM_STRING(prefix, prefix_len)
Z_PARAM_BOOL(more_entropy)
ZEND_PARSE_PARAMETERS_END();

(void)uuidgen(&uuid, 1);

if (more_entropy) {
n[1] &= 0xffffff;
uniqid = strpprintf(0, "%s%08x%06x.%08x", prefix, n[0], n[1], n[2]);
} else {
n[1] &= 0xfffff;
uniqid = strpprintf(0, "%s%08x%05x", prefix, n[0], n[1]);
}

RETURN_STR(uniqid);
}
#elif HAVE_GETTIMEOFDAY
ZEND_TLS struct timeval prev_tv = { 0, 0 };

/* {{{ Generates a unique ID */
Expand All @@ -53,6 +87,24 @@ PHP_FUNCTION(uniqid)
Z_PARAM_BOOL(more_entropy)
ZEND_PARSE_PARAMETERS_END();

#ifdef __NetBSD__
struct uuid uuid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused here. uuidgen is on FreeBSD/NetBSD (other oses have different apis) which you detect at configure time. Why the particular NetBSD code path is needed ? we should fall into the code above I think ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I do not really get the code duplication. But not sure it s worth the efforts to be honest considering later comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be even executed - it's in elif section. Basically:

#ifdef HAVE_UUIDGEN
...
#elif HAVE_GETTIMEOFDAY
...
#ifdef HAVE_UUIDGEN

That doesn't make any sense to me. Am I missing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I committed it a bit too early, I should I removed that section that was used in earlier tests. It is fixed now.

int *n = (int *)&uuid;

/* Use faster uuidgen() if available */
(void)uuidgen(&uuid, 1);

if (more_entropy) {
n[1] &= 0xffffff;
uniqid = strpprintf(0, "%s%08x%06x.%08x", prefix, n[0], n[1], n[2]);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This output format is incompatible with the non-uuidgen output. It also leaks parts of the hosts MAC address and also doesn't actually provide “more entropy”, since the mac address is fixed on a single node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually compatible, despite the fact I do not understand how gettimeofday+more entropy produces 14+8 characters, where I would expect 13+8 reading the code. Check it on your own:
$ php -r 'printf("%s\n", uniqid("", 1));'
67ee7f958a5b34.94453710

For the MAC address, I understand it is not included in UUID anymore:
$ uuidgen
bf0ff2ae-cf7a-46b1-bd2e-c3577268e311
$ uuidgen
39638058-e887-41a5-97d1-6451b0622763

But I can understand it may be platform dependent, and we must avoid using it in case it could contains the MAC.

n[1] &= 0xfffff;
uniqid = strpprintf(0, "%s%08x%05x", prefix, n[0], n[1]);
}

RETURN_STR(uniqid);
#endif

/* This implementation needs current microsecond to change,
* hence we poll time until it does. This is much faster than
* calling usleep(1) which may cause the kernel to schedule
Expand Down
Loading