Roundup Robot added the comment:
New changeset db75553ff333 by Mark Dickinson in branch 'default':
Simplify random_seed to use _PyLong_AsByteArray. Closes issue #16496.
http://hg.python.org/cpython/rev/db75553ff333
--
nosy: +python-dev
___
Python
Changes by Mark Dickinson dicki...@gmail.com:
--
resolution: - fixed
status: open - closed
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
Changes by Andrew Svetlov andrew.svet...@gmail.com:
--
nosy: +asvetlov
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
___
Changes by Jesús Cea Avión j...@jcea.es:
--
nosy: +jcea
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
___
Python-bugs-list mailing list
Mark Dickinson added the comment:
Updated patch: adds in the missing checks for PyMem_Malloc errors.
--
Added file: http://bugs.python.org/file28107/random_seed_metd2.patch
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
Changes by Mark Dickinson dicki...@gmail.com:
--
assignee: - mark.dickinson
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
___
Mark Dickinson added the comment:
Patch looks correct and looks good to me, modulo a couple of nitpicks (see
Rietveld comments). This seems like a nice cleanup.
The patch introduces a new dependence on PY_UINT32_T, which is something we
haven't so far used elsewhere in Python beyond the
Serhiy Storchaka added the comment:
Are there any platforms without 32-bit integers (PY_UINT32_T can be uint32_t,
unsigned int or long)? PyUCS4 also should be 32-bit, therefore Python requires
such type.
--
___
Python tracker rep...@bugs.python.org
Serhiy Storchaka added the comment:
Patch updated to conform with Mark's nitpicks.
What I really doubt is that now same integer seed on little-endian and
big-endian give different random sequences. Is this important? If yes, I can
add bytes-swapping. On other hand, non-integer seed already
Mark Dickinson added the comment:
PyUCS4 also should be 32-bit, therefore Python requires such type.
Hmm, okay. I wonder whether PY_UINT32_T should have been used there, to avoid
doing the same checks in multiple places.
What I really doubt is that now same integer seed on little-endian
Mark Dickinson added the comment:
Thanks for the updated patch. A couple more comments:
- you've got potential overflow when computing keysize from bits, on platforms
where sizeof(size_t) sizeof(unsigned long).
- please could you move the check for PY_UINT32_T nearer the top of the file,
Serhiy Storchaka added the comment:
Patch updated. Now random_seed() is platform-independed for integer arguments.
--
Added file: http://bugs.python.org/file28020/random_seed_3.patch
___
Python tracker rep...@bugs.python.org
Changes by Serhiy Storchaka storch...@gmail.com:
Removed file: http://bugs.python.org/file28020/random_seed_3.patch
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
Serhiy Storchaka added the comment:
Oops, typo.
--
Added file: http://bugs.python.org/file28021/random_seed_3.patch
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
Mark Dickinson added the comment:
I'm still uncomfortable with the init_by_array signature changes and the use of
PY_UINT32_T. How about something like the attached instead? It keeps the
central idea (use _PyLong_NumBits and _PyLong_AsByteArray) but doesn't require
any signature changes or
Serhiy Storchaka added the comment:
I don't think that the preservation of the signature of the auxiliary private
static function is worth it. I'm uncomfortable with such patch. But do as you
feel comfortable.
--
___
Python tracker
Mark Dickinson added the comment:
I'm uncomfortable with such patch.
Any particular reason? It's direct and straightforward, and eliminates the
quadratic behaviour.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
Serhiy Storchaka added the comment:
The code is larger. There is one additional allocation. CPU tacts wasted for
uint32-ulong conversion (and in any case all numbers in the generator are
32-bit). One additional ValeError/OverflowError. Apparently my feeling of
comfort is different from
Mark Dickinson added the comment:
Apparently my feeling of comfort is different from your own. ;)
Yes: I tend to favour direct, readable, and portable code over unnecessarily
optimized code. To address the specific points:
The code is larger.
Very slightly. It's (IMO) more readable and
Antoine Pitrou added the comment:
I agree with Mark, we don't need to micro-optimize here. Also, +1 for not
needing PY_UINT32_T.
--
nosy: +pitrou
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
Serhiy Storchaka added the comment:
I tend to favour direct, readable, and portable code over unnecessarily
optimized code.
And my feeling of directness, readability, and portability also slightly
differs. I agree that code size and additional operations not of importance
here. I say
Stefan Krah added the comment:
Is PY_UINT32_T a big problem? I hope that one day we can use the
C99 types directly. Visual Studio finally supports stdint.h, and
I'm not aware of any compiler that does not.
Consider cdecimal as a trial balloon: It compiles on all obscure
snakebite platforms, and
New submission from Serhiy Storchaka:
Now random_seed() use an ineffective quadratic-time algorithm. It can reuse
_PyLong_AsByteArray(), decreasing code size and increasing performance.
--
components: Extension Modules
files: random_seed.patch
keywords: patch
messages: 175812
nosy:
Martin v. Löwis added the comment:
Why is this a problem?
--
nosy: +loewis
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
___
___
Serhiy Storchaka added the comment:
This is not a problem at all. This just a code cleanup. Removing a code
duplicate. Performance boost is a side effect.
I confused by an old code comment about _PyLong_AsByteArray(). I don't see any
difficulties.
--
Martin v. Löwis added the comment:
Ah. The cleanup looks good, from a shallow glance. I haven't verified yet that
the change is actually correct.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue16496
26 matches
Mail list logo
