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.

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 bpo-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.