[issue19219] speed up marshal.loads()

Changes by Jesús Cea Avión : -- nosy: +jcea ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: In this case, we can remove a bunch of 'retval = NULL' from the code. -- ___ Python tracker ___

[issue19219] speed up marshal.loads()

Roundup Robot added the comment: New changeset 2a2b339b6b59 by Christian Heimes in branch 'default': Issue #19219: retval may be used uninitialized value http://hg.python.org/cpython/rev/2a2b339b6b59 -- ___ Python tracker

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: I've now committed the latest patch (marshal_opts5.patch). -- resolution: -> fixed stage: patch review -> committed/rejected status: open -> closed ___ Python tracker

[issue19219] speed up marshal.loads()

Roundup Robot added the comment: New changeset 4059e871e74e by Antoine Pitrou in branch 'default': Issue #19219: Speed up marshal.loads(), and make pyc files slightly (5% to 10%) smaller. http://hg.python.org/cpython/rev/4059e871e74e -- nosy: +python-dev ___

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: > (however, a quick test suggests that PyUnicode_DecodeUTF8 is quite slower) It's surprising that PyUnicode_DecodeUTF8() is quite slower than _PyUnicode_FromUCS1(). _PyUnicode_FromUCS1() calls ucs1lib_find_max_char() and then memcpy(). PyUnicode_DecodeUTF8() f

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > Let a code say instead me. I don't understand your patch. The macros you define aren't used anywhere. Also, your patch keeps the slow call to PyUnicode_DecodeUTF8. -- ___ Python tracker

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: Let a code say instead me. -- Added file: http://bugs.python.org/file32049/marshal_opts5a.patch ___ Python tracker ___ ___

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: Updated patch using PyUnicode_FromKindAndData for ASCII strings, to ensure that corrupt marshal bytecode doesn't provide corrupt unicode objects. Performance is within 2% of the previous patch. (however, a quick test suggests that PyUnicode_DecodeUTF8 is quite

[issue19219] speed up marshal.loads()

Changes by Brett Cannon : -- nosy: +brett.cannon ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.p

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > > You cannot change the meaning of TYPE_UNICODE (it uses > > "surrogatepass"). > > Therefore, you have to use new opcodes with other semantics. > > You don't need new semantic. Use old semantic. The set of ASCII > encoded strings is a subset of valid UTF8 enco

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: > You cannot change the meaning of TYPE_UNICODE (it uses "surrogatepass"). > Therefore, you have to use new opcodes with other semantics. You don't need new semantic. Use old semantic. The set of ASCII encoded strings is a subset of valid UTF8 encoded strings

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > I meant two of new proposed opcodes: TYPE_ASCII and > TYPE_ASCII_INTERNED. You cannot change the meaning of TYPE_UNICODE (it uses "surrogatepass"). Therefore, you have to use new opcodes with other semantics. Besides, opcodes are cheap. -- _

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: > That would be a change in behaviour, since currently "surrogatepass" is the error handler. I don't propose any change in behaviour. -- ___ Python tracker _

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: > "This will save you two opcodes and perhaps several lines of code. " > Just bear in mind that without other changes, version 4 needs to be backwards > compatible with version 3. I meant two of new proposed opcodes: TYPE_ASCII and TYPE_ASCII_INTERNED. -

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: How about adding a version opcode? This is a backwards compatible change, and allows us to reject unsupported versions in the future, as long as they are not very old unsupported versions. -- ___ Python tra

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > I ran into this when developing version 3. The reason is that while > the marshal format includes the version information in its header, > it isn't actually verified on loading. IIRC. You specify the > expected format to the function, or something like that.

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > > That said, I'll try out the patch with _PyUnicode_FromUCS1 instead > > of _PyUnicode_FromASCII, to see if it affects performance. > > Could you try out the patch with PyUnicode_DecodeUTF8()? This will > save you two opcodes and perhaps several lines of code.

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: "This will save you two opcodes and perhaps several lines of code. " Just bear in mind that without other changes, version 4 needs to be backwards compatible with version 3. I ran into this when developing version 3. The reason is that while the marsha

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: > I don't understand how _PyUnicode_UTF8() can be used for *unmarshalling*. I say about marshalling. > That said, I'll try out the patch with _PyUnicode_FromUCS1 instead of > _PyUnicode_FromASCII, to see if it affects performance. Could you try out the patc

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: Just for the record, I want to say that this is great stuff, Antoine! It's great when this sort of stuff gets some attention. -- ___ Python tracker __

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: That said, I'll try out the patch with _PyUnicode_FromUCS1 instead of _PyUnicode_FromASCII, to see if it affects performance. -- ___ Python tracker ___

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > Sorry for sarcasm. Well, indeed, the sarcasm is undeserved here, if the interpreter cannot crash because of the change. > > It's exactly what you suggest: reuse PyUnicode_AsUTF8String(). > > Actually _PyUnicode_UTF8(). PyUnicode_AsUTF8String() creates UTF8 >

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: > The marshal module is not intended to be secure against erroneous or > maliciously constructed data. Never unmarshal data received from an untrusted > or unauthenticated source. Then we can simplify the marshal module by dropping all error handling: f.rea

[issue19219] speed up marshal.loads()

Changes by Ronald Oussoren : -- nosy: +ronaldoussoren ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://m

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: Anyway, whether or not Pyhon guarantees this and that wrt. "untrusted" is beside the point and offtopic, as Victor poitns out. However: We are, and have always been, careful to fail gracefully if we detect data corruption. Never should the flipping o

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: "Loading untrusted data ... is not supported by Python." This is a pretty bold claim. Is this, indeed, a fact? (and yes, "elsewhere" is a word) -- ___ Python tracker ___

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: "Therefore, the Python maintainers reserve the right to modify the marshal format in backward incompatible ways" sure, don't expect such things to survive version changes. (Actually, they have been hitherto, and my version "3" I actually changed, to be

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: > please discuss it elsewhere. Hum, I'm not sure that this word exist, I mean: somethere else :-) -- ___ Python tracker ___ ___

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: marshal and pickle are unsafe, even without the patch attached to the issue. If you consider that it is an issue that should be fixed, please open a new issue. Antoine's patch doesn't make the module less secure, since it was already not secure :) Loading unt

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > In fact, using 'marshal' as a cheap and fast pickler for builtin types > is actually a good idea because it has no side effects like invoking > code. It's an unsupported use case. The marshal docs are quite clear: """Therefore, the Python maintainers reserve

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: "As for output, we could use cached UTF-8 representation of string (always exists for ASCII only strings) before calling PyUnicode_AsUTF8String()." PyUnicode_AsEncodedString(v, "utf8", "surrogatepass") is expensive. I proposed an optimization for the pickle mo

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: We have to make two distinctions here: 1) Loading data and then running it. This is a bad idea if your data is not trusted. This is what is meant by "marshal" being unsafe. 2) Loading data and then not running it. This is perfectly fine, because marsh

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: "You should ensure that loaded bytes are ASCII-only. Otherwise broken or malicious marshalled data will compromise you program." This is not new, see the red warning in marshal doc: """ Warning The marshal module is not intended to be secure against erroneous

[issue19219] speed up marshal.loads()

Serhiy Storchaka added the comment: > - unmarshalling ASCII strings is faster: you can pass 127 to PyUnicode_New > without scanning for non-ASCII chars You should ensure that loaded bytes are ASCII-only. Otherwise broken or malicious marshalled data will compromise you program. Decoding UTF-8

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: Right, in this case, memory copying is avoided. Regarding the memoing of 0, empty tuple, etc: Special opcode may still benefit because it takes only one byte. So, you save four bytes for each such case. I think it might be worth investigating. --

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > > Reading the code, I see we now maintain a small internal buffer in > > the file object, rather than using stack allocation at the call > > sites. It is unclear to me how this saves memory, since the amount > > of memory copying should be the same. > > No, m

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > This looks fine. Can you quantify where the speedup comes from? >From all changes, but mainly the ASCII special-casing and the new buffering. > Reading the code, I see we now maintain a small internal buffer in > the file object, rather than using stack allo

[issue19219] speed up marshal.loads()

Kristján Valur Jónsson added the comment: I imagine that the test for ASCII is cheaper. It corresponds to the new compact internal unicode representation (one byte characters). This looks fine. Can you quantify where the speedup comes from? Reading the code, I see we now maintain a small int

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: > unmarshalling ASCII strings is faster: you can pass 127 to PyUnicode_New > without scanning for non-ASCII chars Oh, I forgot this pain of the PEP 393. Don't tell me more, it's enough :-) -- ___ Python tracker

[issue19219] speed up marshal.loads()

Antoine Pitrou added the comment: > Why adding ASCII strings, whereas you can add Latin1 (UCS1, U+-U+00FF) > strings? Reasons: - most strings in pyc files are pure ASCII (it's like 99% in the stdlib) - unmarshalling ASCII strings is faster: you can pass 127 to PyUnicode_New without scanning

[issue19219] speed up marshal.loads()

STINNER Victor added the comment: Why adding ASCII strings, whereas you can add Latin1 (UCS1, U+-U+00FF) strings? -- nosy: +haypo ___ Python tracker ___

[issue19219] speed up marshal.loads()

Changes by Antoine Pitrou : -- nosy: +kristjan.jonsson ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://

[issue19219] speed up marshal.loads()

New submission from Antoine Pitrou: This patch contains assorted improvements for unmarshalling pyc files. It will also make them ~10% smaller. $ ./python -m timeit -s "import marshal; d=marshal.dumps(tuple((i, str(i)) for i in range(1000)))" "marshal.loads(d)" -> 3.4 unpatched: 232 usec per