Created on 2008-10-16 22:08 by bob.ippolito, last changed 2022-04-11 14:56 by admin. This issue is now closed.
simplejson 2.0.3 includes major performance enhancements (both in Python and C) and removes usage of the private sre functionality. I will need some help with 3.0 since I am not well versed in the changes to the C API or Python code for that, but merging for 2.6.1 should be no big deal. How should I go about this?
Can you write a patch against python trunk ? :-)
Sure, but that doesn't port it to Python 3.0 :)
> Sure, but that doesn't port it to Python 3.0 :) Still, as Victor suggests, the first step for porting it to 3.0 definitely is to produce a patch for the trunk. What the next steps will be can be discussed when this step has been completed.
patch to r66961 of trunk is attached.
About the patch: are those lines really needed? + PyScannerType.tp_getattro = PyObject_GenericGetAttr; + PyScannerType.tp_setattro = PyObject_GenericSetAttr; + PyScannerType.tp_alloc = PyType_GenericAlloc; + PyScannerType.tp_new = PyType_GenericNew; + PyScannerType.tp_free = _PyObject_Del; I've never used them. What happens if the slots are left empty, and let PyType_Ready() do the rest?
You're probably right, I don't remember what code I was using as a template for that.
Actually, if I remove those lines from the equivalent module in simplejson
it no longer works properly with Python 2.5.2.
File "/Users/bob/src/simplejson/simplejson/decoder.py", line 307, in
__init__
self.scan_once = make_scanner(self)
TypeError: cannot create 'make_scanner' instances
> Actually, if I remove those lines from the equivalent module in simplejson > it no longer works properly with Python 2.5.2. Why aren't the functions pointers in the structs itself? As a procedural note, it seems like this patch is a complete rewrite of the module. Do you anticipate further complete rewrites within the next year? If yes, we should close the issue, and wait for the module to evolve. If no, I'll try to find some time to review the entire module - can you then please post the code to Rietveld?
I don't recall exactly why they aren't in the struct itself, it may not have worked with some compiler on some platform. It's not really a complete rewrite, the encoding path is largely the same and the tests haven't changed. Anyway, there is no further work planned for simplejson. It's done except for the potential for bug fixes. The only enhancements were performance related and this is about as fast as it's going to get. The majority of this work was ready before Python 2.6 was released but it was frozen so I couldn't get this in.
http://codereview.appspot.com/7311
Attached is a new diff, one byte fix to the float parser when parsing JSON documents that are just a float (also a test and a version bump).
Bumping priority a bit.
http://codereview.appspot.com/7311/diff/1/8 File Lib/json/decoder.py (right): http://codereview.appspot.com/7311/diff/1/8#newcode55 Line 55: def py_scanstring(s, end, encoding=None, strict=True, _b=BACKSLASH, _m=STRINGCHUNK.match): This function should get some comments what all the various cases are (preferably speaking with the terms of JSON spec, i.e. chars, char, ...) http://codereview.appspot.com/7311/diff/1/8#newcode71 Line 71: _append(content) # 3 cases: end of string, control character, escape sequence http://codereview.appspot.com/7311/diff/1/8#newcode76 Line 76: msg = "Invalid control character {0!r} at".format(esc) esc isn't assigned until a few lines later. Is this really correct? http://codereview.appspot.com/7311/diff/1/8#newcode104 Line 104: raise ValueError No message? http://codereview.appspot.com/7311/diff/1/8#newcode107 Line 107: raise ValueError No message? http://codereview.appspot.com/7311/diff/1/8#newcode111 Line 111: m = unichr(uni) What's the purpose of m? http://codereview.appspot.com/7311/diff/1/8#newcode127 Line 127: nextchar = s[end:end + 1] Why not s[end]? Add comment if this is necessary. http://codereview.appspot.com/7311/diff/1/8#newcode132 Line 132: nextchar = s[end:end + 1] Likewise. There are more places where it does slicing, but also places where it does indexing, in this function. http://codereview.appspot.com/7311/diff/1/8#newcode290 Line 290: following strings: -Infinity, Infinity, NaN. This sounds like an incompatible change. http://codereview.appspot.com/7311/diff/1/8#newcode317 Line 317: def raw_decode(self, s, idx=0): That looks like an incompatible change http://codereview.appspot.com/7311/diff/1/9 File Modules/_json.c (right): http://codereview.appspot.com/7311/diff/1/9#newcode196 Line 196: output_size *= 2; You might want to check for integer overflow here. http://codereview.appspot.com/7311/diff/1/9#newcode215 Line 215: ascii_escape_str(PyObject *pystr) Please attach a comment to each function, telling what the function does. http://codereview.appspot.com/7311/diff/1/9#newcode733 Line 733: "..." Some text should probably be added here. http://codereview.appspot.com/7311/diff/1/9#newcode1320 Line 1320: if ((idx + 3 < length) && str[idx + 1] == 'u' && str[idx + 2] == 'l' && str[idx + 3] == 'l') { Is this really faster than a strncmp? http://codereview.appspot.com/7311/diff/1/9#newcode1528 Line 1528: PyTypeObject PyScannerType = { I think scanner objects should participate in cyclic gc. http://codereview.appspot.com/7311/diff/1/9#newcode2025 Line 2025: "make_encoder", /* tp_name */ That is a confusing type name. How about "Encoder"? http://codereview.appspot.com/7311
By "next patch" I'm referring to a currently nonexistent patch that would merge the json library with simplejson 2.0.7 (svn trunk at the moment). I may have time to create it next weekend. --- http://codereview.appspot.com/7311/diff/1/8 File Lib/json/decoder.py (right): http://codereview.appspot.com/7311/diff/1/8#newcode55 Line 55: def py_scanstring(s, end, encoding=None, strict=True, _b=BACKSLASH, _m=STRINGCHUNK.match): Commented in the next patch. http://codereview.appspot.com/7311/diff/1/8#newcode71 Line 71: _append(content) Commented in the next patch http://codereview.appspot.com/7311/diff/1/8#newcode76 Line 76: msg = "Invalid control character {0!r} at".format(esc) This is a bug in the exception handling code, fixed in the next patch. http://codereview.appspot.com/7311/diff/1/8#newcode104 Line 104: raise ValueError Exception is caught at the except block and re-raised with a message. Next patch unrolls this so it's not confusing. http://codereview.appspot.com/7311/diff/1/8#newcode107 Line 107: raise ValueError Exception is caught at the except block and re-raised with a message. Next patch unrolls this so it's not confusing. http://codereview.appspot.com/7311/diff/1/8#newcode111 Line 111: m = unichr(uni) Renamed m to char in the next patch. http://codereview.appspot.com/7311/diff/1/8#newcode127 Line 127: nextchar = s[end:end + 1] commented in next patch (only once). s[end] can raise an IndexError with bad input, s[end:end+1] returns an empty string on underflow, which is caught later with a more helpful error message. http://codereview.appspot.com/7311/diff/1/8#newcode132 Line 132: nextchar = s[end:end + 1] commented in next patch (only once). s[end] can raise an IndexError with bad input, s[end:end+1] returns an empty string on underflow, which is caught later with a more helpful error message. http://codereview.appspot.com/7311/diff/1/8#newcode290 Line 290: following strings: -Infinity, Infinity, NaN. Not practically speaking. The documented purpose of this callback is "This can be used to raise an exception if invalid JSON numbers are encountered.". I've never seen it used to handle None, True, False in a different manner. That was more of an implementation detail than anything else, and that is fixed by this patch. Existing implementations of this callback will simply have dead code since they will never be called with "null", "true" or "false" anymore. http://codereview.appspot.com/7311/diff/1/8#newcode317 Line 317: def raw_decode(self, s, idx=0): It is a compatible change. http://codereview.appspot.com/7311/diff/1/9 File Modules/_json.c (right): http://codereview.appspot.com/7311/diff/1/9#newcode196 Line 196: output_size *= 2; _PyString_Resize checks for integer overflow, so it would explode there just fine. The next patch changes this slightly to avoid unnecessary calls to _PyString_Resize when the size didn't actually change, but doesn't do any explicit integer overflow checking http://codereview.appspot.com/7311/diff/1/9#newcode215 Line 215: ascii_escape_str(PyObject *pystr) Done in the next patch http://codereview.appspot.com/7311/diff/1/9#newcode733 Line 733: "..." Done in the next patch. http://codereview.appspot.com/7311/diff/1/9#newcode1320 Line 1320: if ((idx + 3 < length) && str[idx + 1] == 'u' && str[idx + 2] == 'l' && str[idx + 3] == 'l') { Probably not, but strncmp doesn't work for PyUnicode and the same code is repeated there. http://codereview.appspot.com/7311/diff/1/9#newcode1528 Line 1528: PyTypeObject PyScannerType = { I don't think it's possible to cause a cycle using the documented APIs, since the encoder is created and thrown away behind the scenes and never passed to user code. Someone else can write that patch if it's necessary. http://codereview.appspot.com/7311/diff/1/9#newcode2025 Line 2025: "make_encoder", /* tp_name */ It's not a type that's ever exposed to user code, make_encoder is somewhat less confusing because that's the name it's exposed as. I'll change it anyway though, it doesn't really matter since this is all private API.
Bob, any news on this?
patch to r69662 is attached as json_issue4136_r69662.diff -- note that simplejson 2.0.9 isn't released, as of r169 it's just simplejson 2.0.8 with some trivial changes to make this backport easier for me
A bunch of comments from a quick look:
- why do you use old-style relative imports ("from decoder import
JSONDecoder")?
- in join_list_unicode, join_list_string you could use PyUnicode_Join
and _PyString_Join, respectively
- in scanstring_unicode, the top comment says "encoding is the encoding
of pystr (must be an ASCII superset)", but the function takes no
"encoding" parameter
- there are some lines much longer than 80 chars (it's quite clear when
reading the diff)
- there are places where you call PyObject_IsTrue(s->strict) without
checking for an error return; perhaps you could do so in the constructor
- the Scanner type doesn't support cyclic garbage collection, but it
contains some arbitrary Python objects (parse_constant and friends could
be closures or methods)
- same issue with the Encoder type (default_fn could hold arbitrary
objects alive)
Bob, here is a small example showing how easy it is to encounter the GC
problem:
from json import JSONDecoder
import weakref
import gc
class MyObject(object):
def __init__(self):
self.decoder = JSONDecoder(parse_constant=self.parse_constant)
def parse_constant(self, *args, **kargs):
""" XXX """
wr = weakref.ref(MyObject())
gc.collect()
print wr()
Old-style relative imports and the way that it does join are there because it's Python 2.4 compatible code that I'm porting. I'll add those to the list of things that need to be changed when backporting, implement cyclic GC on the types, and I'll take a look at lines > 80 chars and fix any that occur in Python code (though for some of the tests it may be a bit more effort than its worth). It will probably take another week or two for me to implement those things and then do another backport.
New patch implementing cyclic GC, new-style relative imports, no lines >80 characters in non-test Python code
Thanks for the update. I'll try to take the time for a review in less than one month. In the meantime, though, I want to point out that the 80-character rule should also apply to C files. You have quite a bit of huge C code lines, especially in parsing code.
Honestly I'm not sure when I'm going to find the time and motivation to reformat the C source and tests to fit < 80 char lines. I don't think this should hold up the patch, someone who is more obsessive compulsive than myself can fix that once it hits trunk :)
I think reformatting line length should not hold-up this patch. That is a nice-to-have, not a must-have.
Reviewers: , Description: Updated patch from Bob Ippolito, for updating the Python trunk json package to the latest simplejson. Please review this at http://codereview.appspot.com/20095 Affected files: Lib/json/__init__.py Lib/json/decoder.py Lib/json/encoder.py Lib/json/scanner.py Lib/json/tests/test_check_circular.py Lib/json/tests/test_decode.py Lib/json/tests/test_dump.py Lib/json/tests/test_encode_basestring_ascii.py Lib/json/tests/test_fail.py Lib/json/tests/test_float.py Lib/json/tests/test_unicode.py Lib/json/tool.py Modules/_json.c
FWIW, following simplejson's SVN history[1] makes understanding the (bits of the) patch (that I had time to look at) much easier to me. I recall other JSON packages having lots of cornercase tests, not sure if they'd be relevant here. But sprinkling a few more tests around might help digest these changes :) [1] http://code.google.com/p/simplejson/source/list?start=123
The one thing that IMO needs to be decided before this can be accept is the version compatibility: what Python versions must this code stay compatible with? That decision then needs to be implemented. Apart from this (and the additional minor comments below), the patch looks fine. http://codereview.appspot.com/20095/diff/1/13 File Lib/json/decoder.py (right): http://codereview.appspot.com/20095/diff/1/13#newcode21 Line 21: nan, inf = struct.unpack('dd', _BYTES) I think this can be simplified as nan, inf = struct.unpack('>dd', _BYTES) http://codereview.appspot.com/20095/diff/1/12 File Lib/json/encoder.py (right): http://codereview.appspot.com/20095/diff/1/12#newcode31 Line 31: INFINITY = float('1e66666') Why not decoder.PosInf? http://codereview.appspot.com/20095/diff/1/14 File Modules/_json.c (right): http://codereview.appspot.com/20095/diff/1/14#newcode3 Line 3: #if PY_VERSION_HEX < 0x02060000 && !defined(Py_TYPE) Is Python before 2.6 even supported anymore? ISTM that the usage of .format on strings outrules Python2.5 and earlier. http://codereview.appspot.com/20095
> simplejson maintains Python 2.4+ compatibility, but json maintains 2.6+. > I could produce another patch that manually removes these few remaining > nits if it's necessary. I don't quite understand this: isn't json/decoder.py and simplejson/decoder.py essentially the same? why fork the one and not the other? However, as long as the compatibility requirements are documented somewhere (e.g. PEP 291), it's fine with me.
They are essentially the same except the relative imports are changed to use . syntax, simplejson._speedups is changed to _json, simplejson is changed to json, .format strings are used, and the test suite changes slightly. I can add fixing that struct function and removing the #if stuff from the C code to that list as well. The way I see it, the names have to change anyway, so other things might as well be modernized as long as it's trivial. I personally didn't make the call to switch from % to .format, someone else did that after I had originally committed simplejson to Python 2.6 trunk.
Martin, is this patch good-to-go?
What needs to happen next for this patch to go forward?
Well, if Bob has addressed all of Martin's comments, I suppose it can get in. The second step will be to port it to py3k...
All of the comments are addressed. I am not going to go through the trouble of creating a new patch to remove the remaining backwards compatibility cruft in the C code and struct function. That is easier to remove later.
The patch in its current form is fine with me, please apply (OTOH, I don't see the need for urgency - 2.7 is still many months away, and likely, we will see another update to the same code before it gets released)
Bob, please go ahead and commit. I don't see any advantage to letting the code continue sit in the tracker. Also, having it in will let me go forward with issue 5381 which has been held-up until this was complete. Thanks for all your work on JSON.
r70443 in trunk
Reopening so that we don't forget to merge it in py3k :) (I have the feeling it won't be trivial, although I hope to be proven wrong)
It might not be bad. I read through the patch a saw that it uses only the most basic C APIs and already has unicode aware sections. It may be a matter of switching the PyString functions. Will look at it in more detail later this week. Fortunately, there is not a lot of C code.
This change should be ported to py3k sometime before the first beta.
Here's a half-baked patch against py3k. It resolves all the conflicts but still has 15 failing tests. Perhaps someone would like to finish it up. For example, json.dumps(b"hi") works, but not json.dumps([b"hi", "hi"])
There is the problem in the current py3k version of json. b"hi" can be serialized, but not [b"hi"]. >>> json.dumps(b"hi") '"hi"' >>> json.dumps([b"hi"]) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/antoine/py3k/__svn__/Lib/json/__init__.py", line 230, in dumps return _default_encoder.encode(obj) File "/home/antoine/py3k/__svn__/Lib/json/encoder.py", line 367, in encode chunks = list(self.iterencode(o)) File "/home/antoine/py3k/__svn__/Lib/json/encoder.py", line 306, in _iterencode for chunk in self._iterencode_list(o, markers): File "/home/antoine/py3k/__svn__/Lib/json/encoder.py", line 204, in _iterencode_list for chunk in self._iterencode(value, markers): File "/home/antoine/py3k/__svn__/Lib/json/encoder.py", line 317, in _iterencode for chunk in self._iterencode_default(o, markers): File "/home/antoine/py3k/__svn__/Lib/json/encoder.py", line 323, in _iterencode_default newobj = self.default(o) File "/home/antoine/py3k/__svn__/Lib/json/encoder.py", line 344, in default raise TypeError(repr(o) + " is not JSON serializable") TypeError: b'hi' is not JSON serializable
Christian: 1) in py3k, loads and dumps always seem to operate on/produce str objects, but encode_basestring_ascii returns a bytes object. Why is that? 2) what is the use of the encoding argument in py3k? it looks completely ignored (bytes objects are not allowed as input and never produced as output)
Updated patch: - fixes all failures - removes bytes input and output "support" (which didn't work but still involved a lot of code) To be done: - remove all traces of the encoding argument, and associated machinery
Here is an updated patch, completely removing the `encoding` parameter and fixing docs.
(by the way, all tests pass)
It would be better to have a patch that diff's from the current 2.7 version than to start with the 3.0 version; otherwise, the two will never be fully synchronized and some of the choices made in 2.6-to-3.0 will live on forever. The 2.7 version reflects more patch review and real world usage (from simplejson) than the relatively unexercised 3.0 version.
> It would be better to have a patch that diff's from the current 2.7 > version than to start with the 3.0 version; otherwise, the two will > never be fully synchronized and some of the choices made in 2.6-to-3.0 > will live on forever. How am I supposed to produce this patch?
The idea is to ignore the current 3.0 version and just redo the 2-to-3 conversion from 2.7 and do it well this time. Compute the 3.1 patch as if the current 3.0 version was blown away (reverted).
+1 for Raymond's suggestion The 3.0 version of json was more like a last minute patch work than thorough work. You might wanna svn rm the 3.0 code, svn cp the 2.7 code to the py3k branch and start all over.
I'll take a stab at doing it Raymond's way this weekend.
Since no other patches were proposed, I applied Antoine's patch in r72194.
messages: + msg86942
messages: + msg85775
messages: + msg85759
messages: + msg85752
nosy: + benjamin.peterson
messages: + msg85118
messages:
+ msg83715
versions:
- Python 2.7
messages: + msg83713
messages: + msg83706
messages: + msg82887
messages: + msg82875
messages: + msg82613
messages: + msg82234
nosy: + pitrou
title: merge json library with simplejson 2.0.3 -> merge json library with latest simplejson 2.0.x
messages: + msg82053
type: behavior
stage: needs patch
title: merge json library with simplejson 2.0.4 -> merge json library with simplejson 2.0.3
nosy: + georg.brandl
messages: + msg78697
messages: + msg75171
messages: + msg74931
files: + json_issue4136_r66961.diff
messages: + msg74929
versions: + Python 3.1, Python 2.7
messages: + msg74877
messages: + msg74874