bpo-30977: make uuid.UUID use __slots__ to reduce its memory footprint #9078
Conversation
Based on original patch by Wouter Bolsterlee.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM. You should use "Co-Authored-By: name " syntax in the (final) commit message to credit Wouter Bolsterlee. I just have a minor request on coding style.
Lib/test/test_uuid.py
Outdated
|
|
||
| # Python 2.7 protocol 0-2 pickles of u | ||
| py27_pickles = [ | ||
| b'ccopy_reg\n_reconstructor\np0\n(cuuid\nUUID\np1\nc__builtin__\nobject\np2\nNtp3\nRp4\n(dp5\nS\'int\'\np6\nL24197857161011715162171839636988778104L\nsb.', |
There was a problem hiding this comment.
Can you try to reformat to limit to 80 characters?
|
Thanks for the quick review, @vstinner! |
| self.addCleanup(restore_uuid_module) | ||
|
|
||
| def test_pickle_roundtrip(self): | ||
| self._setup_for_pickle() |
There was a problem hiding this comment.
Wouldn't be easier to use with support.swap_item(sys.modules, 'uuid', self.uuid):?
| self._setup_for_pickle() | ||
|
|
||
| u = self.uuid.UUID('12345678123456781234567812345678') | ||
| self.assertEqual(u, pickle.loads(pickle.dumps(u))) |
There was a problem hiding this comment.
Need to test with all pickle protocols. Need to test shallow and deep copying too, since they use the same protocol.
|
|
||
| u = self.uuid.UUID('12345678123456781234567812345678') | ||
|
|
||
| # Python 2.7 protocol 0-2 pickles of u |
There was a problem hiding this comment.
It would be better to pass pickles through pickletools.optimize() for removing unneeded memoization. This will make the input data shorter.
| object.__setattr__(self, 'is_safe', is_safe) | ||
|
|
||
| def __getstate__(self): | ||
| d = {attr: getattr(self, attr) for attr in self.__slots__} |
There was a problem hiding this comment.
Why use self.__slots__ instead of just hardcoding two attribute names? In any case this code doesn't work in case of subclasses with additional attributes.
If is_safe is SafeUUID.unknown, it can be omitted for saving space.
| # is_safe was added in 3.7 | ||
| state.setdefault('is_safe', SafeUUID.unknown.value) | ||
|
|
||
| for attr in self.__slots__: |
There was a problem hiding this comment.
The code would be simpler if just set int and is_safe attributes.
| def test_unpickle_previous_python_versions(self): | ||
| self._setup_for_pickle() | ||
|
|
||
| u = self.uuid.UUID('12345678123456781234567812345678') |
There was a problem hiding this comment.
Could you use UUID that doesn't contain regular patterns?
| try: | ||
| value = SafeUUID(value) | ||
| except ValueError: | ||
| value = SafeUUID.unknown |
There was a problem hiding this comment.
Why this error is silenced?
There was a problem hiding this comment.
You brought up the need to support pickles from future Python versions; those could potentially define new values in the SafeUUID enum.
| def test_pickle_roundtrip(self): | ||
| self._setup_for_pickle() | ||
|
|
||
| u = self.uuid.UUID('12345678123456781234567812345678') |
There was a problem hiding this comment.
Need to test with different is_safe values. And please used UUID with less regular hexadecimal representation.
| b'\x12xV4\x12xV4\x12sb.', | ||
| ] | ||
|
|
||
| for pickled in py27_pickles + py36_pickles: |
There was a problem hiding this comment.
Why two lists are used?
There was a problem hiding this comment.
To clearly differentiate between those generated with different Python versions, using variable names rather than comments.
|
|
||
| def __getstate__(self): | ||
| d = {attr: getattr(self, attr) for attr in self.__slots__} | ||
| # is_safe is a SafeUUID instance. Return just its value, so that |
There was a problem hiding this comment.
Is the value portable? Seems there is a problem in 3.7 which should be fixed (this is a different issue).
There was a problem hiding this comment.
I'm not sure what you mean. The value is one of None, 0 or -1, so it should be "portable" for any meaning of that word.
We have to keep just the value to avoid errors when unpickling in Python versions <3.7, where the SafeUUID enum isn't defined. Using the .value, is_safe is ignored and unpickling succeeds.
|
Thanks for the in-depth review @serhiy-storchaka! I learned a lot. Since this has already been merged, see new PR #9106 with the appropriate changes. |
5 participants
Based on original patch by Wouter Bolsterlee. (see PR #2785)
Added pickle/unpickle logic to maintain forward and backward compatibility with other Python versions.
Manually tested that unpickling works with Python 3.6 and 2.7.
https://bugs.python.org/issue30977