bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid #3796

pitrou · 2017-09-28T10:37:55Z

https://bugs.python.org/issue11063

https://bugs.python.org/issue20519

vstinner · 2017-09-28T10:46:33Z

Please add a NEWS entry.

pitrou · 2017-09-28T11:11:32Z

The potential issue here is that the uuid headers are not always installed (for example they aren't on the Travis-CI machine, even though it has many development headers installed).

vstinner · 2017-09-28T13:49:40Z

Modules/_uuidmodule.c

+PyMODINIT_FUNC
+PyInit__uuid(void)
+{
+    assert(sizeof(uuid_t) == 16);


You might use Py_BUILD_ASSERT() to run the check at the compilation.

Is sizeof() allowed in macros? Usually we have SIZEOF_* for that.

I don't know. In case of doubt, keep your assert(), it's fine.

vstinner · 2017-09-28T13:50:01Z

Modules/_uuidmodule.c

+
+
+static PyObject *
+_uuid_generate_time_safe(void)


I suggest to add a py prefix: py_uuid_generate_time_safe().

vstinner · 2017-09-28T13:50:41Z

Modules/_uuidmodule.c

+    NULL,
+    -1,
+    uuid_methods,
+    NULL,


Maybe remove trailing "NULL,". The compiler set them to 0 / NULL anyway.

vstinner · 2017-09-28T13:52:34Z

Modules/_uuidmodule.c

+    "_uuid",
+    NULL,
+    -1,
+    uuid_methods,


Can you use C99 style here to avoid surprising/boring "NULL," members?

.m_name = "_uuid", .m_size = -1, .m_methods = uuid_methods,

Oh, great, I'll do that.

vstinner · 2017-09-28T13:54:27Z

Lib/uuid.py

+        else:
+            def _generate_time_safe():
+                return _uuid.generate_time_safe()
+            _has_uuid_generate_time_safe = True


Why not just _generate_time_safe = _uuid.generate_time_safe?

I would prefer an explicit "return" here.

vstinner · 2017-09-28T13:59:34Z

Lib/test/test_uuid.py

+
+@unittest.skipUnless(c_uuid, 'requires the C _uuid module')
+class TestInternalsWithExtModule(BaseTestInternals, unittest.TestCase):
+    uuid = c_uuid


I don't see the point of testing all uuid.*getnode() functions with and without _uuid. Basically, it's the same test run twice, no?

Only getnode() and uuid1() should be tested with and without _uuid, no?

Only getnode() and uuid1() should be tested with and without _uuid, no?

Probably. I didn't want to modify the tests too much but I'll take a look.

I've changed my mind: I think all tests should be run both with and without the C _uuid extension module. It doesn't cost much to do so, and it's more future-proof.

bedevere-bot · 2017-09-28T13:59:42Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2017-09-28T14:00:10Z

@pitrou: "The potential issue here is that the uuid headers are not always installed (for example they aren't on the Travis-CI machine, even though it has many development headers installed)."

That's fine. We have buildbots for further tests.

pitrou · 2017-09-28T17:07:16Z

@Haypo, I think I've addressed your comments.

vstinner

Ah, the new PR now looks better! I hope that it's my last round of minor comments :-)

vstinner · 2017-09-28T19:38:11Z

Modules/_uuidmodule.c

+    PyModuleDef_HEAD_INIT,
+    .m_name = "_uuid",
+    .m_size = -1,
+    .m_methods = uuid_methods,


Oh, I really like this syntax. We should use it more often!

vstinner · 2017-09-28T19:42:08Z

setup.py

@@ -1668,6 +1668,20 @@ class db_found(Exception): pass
        if '_tkinter' not in [e.name for e in self.extensions]:
            missing.append('_tkinter')

+        # Build the _uuid module if possible
+        build_uuid = False
+        if find_file("uuid.h", inc_dirs, ["/usr/include/uuid"]):


Since you add /usr/include/uuid to inc_dirs, I would maybe store find_file() result into uuid_incdirs and then pass it to Extension(): inc_dirs=uuid_incdirs? Similar code is used for ssl_incs.

vstinner · 2017-09-28T19:44:14Z

setup.py

+        # Build the _uuid module if possible
+        build_uuid = False
+        if find_file("uuid.h", inc_dirs, ["/usr/include/uuid"]):
+            if self.compiler.find_library_file(lib_dirs, 'uuid'):


You may also store the result of find_library_file() into uuid_libdirs to pass it to Extension library_dirs. I'm not sure about this one.

Let me try...

No, it shouldn't be necessary. Other uses of find_library_file() don't do it.

vstinner · 2017-09-28T19:49:32Z

Lib/test/test_uuid.py

@@ -14,7 +17,8 @@ def importable(name):
    except:
        return False

-class TestUUID(unittest.TestCase):
+class BaseTestUUID:
+


Please add "uuid = None" here.

vstinner · 2017-09-28T19:49:53Z

Lib/test/test_uuid.py

+    uuid = c_uuid
+
+
+class BaseTestInternals:


Please add "uuid = None" here.

vstinner · 2017-09-28T19:53:29Z

Lib/uuid.py

+                continue
+            # Try to find the safe variety first.
+            if hasattr(lib, 'uuid_generate_time_safe'):
+                _uuid_generate_time = lib.uuid_generate_time_safe


Would you mind to rename "_uuid_generate_time" to "_uuid_generate_time_safe"? I was confused while reading the code.

vstinner · 2017-09-28T20:00:07Z

Lib/uuid.py

+    if _has_uuid_generate_time_safe is not None:
+        return
+
+    _has_uuid_generate_time_safe = False


I'm not sure of the value of _has_uuid_generate_time_safe. The exact value (True or False) is not use. Maybe rename the flag to _system_functions_initialized or something like that, and only use False/True values?

It's used in the tests.

Oh, I didn't notice. In this case, it's fine. I was just surprised to see 3 states where only 2 states are used in uuid.

vstinner · 2017-09-28T20:32:30Z

I hacked the code to disable _uuid, uuid_generate_time_safe and uuid_generate_time in uuid.py. With these changes, tests fail:

======================================================================
ERROR: test_unix_getnode (test.test_uuid.TestInternalsWithExtModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/haypo/prog/python/master/Lib/test/test_uuid.py", line 574, in test_unix_getnode
    self.check_node(self.uuid._unix_getnode(), 'unix')
  File "/home/haypo/prog/python/master/Lib/uuid.py", line 578, in _unix_getnode
    uuid_time, _ = _generate_time_safe()
TypeError: 'NoneType' object is not callable

======================================================================
ERROR: test_unix_getnode (test.test_uuid.TestInternalsWithoutExtModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/haypo/prog/python/master/Lib/test/test_uuid.py", line 574, in test_unix_getnode
    self.check_node(self.uuid._unix_getnode(), 'unix')
  File "/home/haypo/prog/python/master/Lib/uuid.py", line 578, in _unix_getnode
    uuid_time, _ = _generate_time_safe()
TypeError: 'NoneType' object is not callable

Python 3.6 test contains:

        try: # Issues 1481, 3581: _uuid_generate_time() might be None.
            node = uuid._unixdll_getnode()
        except TypeError:
            self.skipTest('requires uuid_generate_time')

pitrou · 2017-09-28T20:33:06Z

You're right, I'm gonna reinstate the try / except clause.

vstinner

LGTM as soon as you fix the test failure I reported in my latest comment.

igalic · 2017-10-21T17:30:13Z

Is it possible to get this back-ported into (i guess, at least) 3.6?

vstinner · 2017-10-21T23:40:59Z

No, sorry, it's not possible. The change is too big to be qualified as a bugfix. For me, it's a new feature and so cannot be backported. The risk of regression is too high. The best you can do is to write a 3rd party extension on PyPI to backport the module to 3.6 and maybe older (2.7??).

bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid

32b1b40

bedevere-bot added the awaiting merge label Sep 28, 2017

the-knights-who-say-ni added the CLA signed label Sep 28, 2017

pitrou added 2 commits September 28, 2017 13:16

Fix _windll_getnode

05dcee2

Add blurb

f4d7cf3

vstinner mentioned this pull request Sep 28, 2017

bpo-11063: Create _uuid1 submodule #3795

Closed

vstinner requested changes Sep 28, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 28, 2017

vstinner mentioned this pull request Sep 28, 2017

bpo-5885: fix slow uuid initialization #3684

Closed

pitrou added 3 commits September 28, 2017 18:17

Nits in uuid.py

8f4dfb2

Generalize testing with and without C extension "_uuid"

a78dc43

Fix tests on Windows

34df739

vstinner reviewed Sep 28, 2017

View reviewed changes

Address review comments

92b8670

vstinner approved these changes Sep 28, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 28, 2017

Fix test regression

88bb338

vstinner mentioned this pull request Sep 28, 2017

bpo-29729: uuid.UUID now accepts bytes-like object #3801

Closed

pitrou merged commit a106aec into python:master Sep 28, 2017

pitrou deleted the uuid_ext_module branch September 28, 2017 21:03

Mariatta removed the awaiting merge label Oct 8, 2017

adamantike mentioned this pull request Aug 4, 2019

import redis slow on FreeBSD / Python3.6 / related to uuid redis/redis-py#1193

Closed

sobolevn mentioned this pull request Dec 20, 2023

uuid has a bunch of deprecated functions: let's decide what to do with them #113308

Closed



		static PyObject *
		_uuid_generate_time_safe(void)

bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid #3796

bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid #3796

Conversation

pitrou commented Sep 28, 2017 • edited Loading

vstinner commented Sep 28, 2017

pitrou commented Sep 28, 2017

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

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

bedevere-bot commented Sep 28, 2017

vstinner commented Sep 28, 2017

pitrou commented Sep 28, 2017

vstinner 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

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

Choose a reason for hiding this comment

vstinner commented Sep 28, 2017

pitrou commented Sep 28, 2017

vstinner left a comment

Choose a reason for hiding this comment

igalic commented Oct 21, 2017

vstinner commented Oct 21, 2017 via email

pitrou commented Sep 28, 2017 •

edited

Loading