homepage

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.