gh-95382: Improve performance of json encoder with indent #118105

eendebakpt · 2024-04-19T15:18:28Z

In this PR we implement the indent argument for the C implemention of the JSON encoder.

Benchmark:

json.dumps indent=4: Mean +- std dev: [main] 27.0 us +- 1.5 us -> [pr] 11.0 us +- 0.6 us: 2.45x faster
json.dumps: Mean +- std dev: [main] 8.90 us +- 0.30 us -> [pr] 8.73 us +- 0.35 us: 1.02x faster

Geometric mean: 1.58x faster

Updated benchmark after using PySequence_Repeat :

json.dumps indent=4: Mean +- std dev: [main] 27.0 us +- 1.5 us -> [pr_v2] 10.3 us +- 0.3 us: 2.62x faster
json.dumps: Mean +- std dev: [main] 8.90 us +- 0.30 us -> [pr_v2] 9.02 us +- 0.37 us: 1.01x slower

Geometric mean: 1.61x faster

Benchmark script

import json
import pyperf

obj = {'list': [1, 2, 3, 4, 5., 6., 7.], 'dict': {'a': 'A', 'b': 'A', 'c': 'C'},
       'tuple': tuple(ii**2/3 for ii in range(20)), 'bool': True, 'float': float('nan'), }

json_dumps = lambda data: json.dumps(data)
json_dumps4 = lambda data: json.dumps(data, indent=4)

runner = pyperf.Runner()
runner.bench_func('json.dumps indent=4', json_dumps4, obj)
runner.bench_func('json.dumps', json_dumps, obj)

Notes

The implementation of _create_newline_indent is not very efficient. We can improve that either in a followup PR or in this PR by adding a cache (probably with python dict). Even without this improvement the C implementation is now much faster than the Python implementation.
We keep the Python implementation of the encoder unchanged (https://peps.python.org/pep-0399/)

Issue: Remove unused indent_level from Modules/_json.c #95382

Modules/_json.c

Co-authored-by: Nice Zombies <[email protected]>

JelleZijlstra · 2024-04-21T02:05:58Z

Modules/_json.c

+    PyObject* start = PyUnicode_FromString("\n");
+    PyObject* newline_indent = NULL;
+    if (start == 0) {
+        goto end;


mul_name will be used uninitialized if this branch is taken.

Also, == 0 looks odd for pointer comparisons; compare against NULL instead.

JelleZijlstra · 2024-04-21T02:07:24Z

Modules/_json.c

+    if (start == 0) {
+        goto end;
+    }
+    // there is no public PyUnicode_Repeat?


Does https://docs.python.org/3/c-api/sequence.html#c.PySequence_Repeat work?

Yes, great suggestion!

Modules/_json.c

JelleZijlstra · 2024-04-21T02:10:25Z

Modules/_json.c

+            goto bail;
+        }
+
+        separator = PyUnicode_Concat(separator, newline_indent);


I think this leaks a reference to the previous contents of separator.

The previous contents of separator was a borrowed reference, so it should be fine. But correct me if I am wrong!

nineteendo · 2024-04-21T19:12:50Z

Don't forget to resolve the conversations.

nineteendo · 2024-04-21T19:15:32Z

Could you also configure pre-commit? https://devguide.python.org/getting-started/setup-building/#install-pre-commit

nineteendo · 2024-04-21T19:19:00Z

It blocks you from committing trailing whitespace:

diff --git a/Modules/_json.c b/Modules/_json.c
index 7bc23d1..828c35c 100644
--- a/Modules/_json.c
+++ b/Modules/_json.c
@@ -1360,7 +1360,7 @@ _steal_accumulate(_PyUnicodeWriter *writer, PyObject *stolen)
 PyObject* _create_newline_indent(PyObject* indent, Py_ssize_t indent_level)
 {
     PyObject* newline_indent = NULL;
- 
+
     PyObject* _current_indent = PySequence_Repeat(indent, indent_level);
     if (_current_indent == NULL) {
         goto end;

Modules/_json.c

eendebakpt · 2024-04-21T19:45:09Z

Don't forget to resolve the conversations.

Typically I leave that to the reviewers (some reviewers prefer it that way)

nineteendo · 2024-04-21T19:49:20Z

Also note that regular reviewers (like me) can't resolve conversations. So, you (or someone else) willl have to resolve that conversation.

eendebakpt · 2024-04-21T20:04:15Z

Also note that regular reviewers (like me) can't resolve conversations. So, you (or someone else) willl have to resolve that conversation.

Ah, I did not know that. Well, we can agree among the two of us it is ok to resolve each others comments when addressed.

Modules/_json.c

JelleZijlstra · 2024-04-21T21:04:40Z

Modules/_json.c

+
+    PyObject* _current_indent = PySequence_Repeat(indent, indent_level);
+    if (_current_indent == NULL) {
+        goto end;


This goto will use start before it is defined.

Initialise it with NULL like newline_indent.

Or just return NULL.

Modules/_json.c

JelleZijlstra · 2024-04-21T21:07:34Z

Modules/_json.c

+            goto bail;
+        }
+
+        separator = PyUnicode_Concat(separator, newline_indent); // non-borrowed reference


I don't know that "non-borrowed reference" means here but it looks like we're leaking a reference to the previous contents of separator.

It is okay in normal case. separator is a borrowed reference when s->indent == Py_None and a non-borrowed reference when s->indent != Py_None. It is balanced out in normal flow. But if an error happens and we go to the bail label, separator can be leaked. It should be decrefed if needed.

Lib/json/encoder.py

Modules/_json.c

serhiy-storchaka · 2024-04-22T14:46:14Z

Modules/_json.c

+
+    PyObject* _current_indent = PySequence_Repeat(indent, indent_level);
+    if (_current_indent == NULL) {
+        goto end;


Or just return NULL.

Modules/_json.c

serhiy-storchaka · 2024-04-22T15:16:17Z

Modules/_json.c

+            goto bail;
+        }
+
+        separator = PyUnicode_Concat(separator, newline_indent); // non-borrowed reference


It is okay in normal case. separator is a borrowed reference when s->indent == Py_None and a non-borrowed reference when s->indent != Py_None. It is balanced out in normal flow. But if an error happens and we go to the bail label, separator can be leaked. It should be decrefed if needed.

serhiy-storchaka · 2024-04-22T15:26:30Z

Modules/_json.c

-        indent_level -= 1;
+        indent_level--;
+        Py_DECREF(separator);
+        newline_indent = _create_newline_indent(s->indent, indent_level);


newline_indent is calculated twice per every list or dict. First for nested items, second for the closing ] or }. The latter is the same as newline_indent for items of the outer list or dict.

In the following example the indent for the nested ] is the same as the indent for 1, 2 and the nested [:

[ 1, 2, [ 3, 4 ] ]

I think that we can just pass it as an argument of encoder_listencode_list() and encoder_listencode_dict().

The inner value can then be calculated as a simple concatenation, without multiplication:

item_newline_indent = PyUnicode_Concat(newline_indent, s->ident);

Modules/_json.c

nineteendo · 2024-04-24T23:03:26Z

Sorry, I accidentally added these as single comments. I thought I was still editing my review.

Modules/_json.c

Co-authored-by: Nice Zombies <[email protected]>

Lib/json/encoder.py

Modules/_json.c

serhiy-storchaka · 2024-04-25T14:05:34Z

Modules/_json.c

+            return NULL;
+        }
+    }
+    if (encoder_listencode_obj(self, &writer, obj, indent_level,


Do we need to pass indent_level if we pass current_newline_indent? I suppose that it is no longer used.

serhiy-storchaka · 2024-04-25T14:06:51Z

Modules/_json.c

+                         PyObject *current_newline_indent,
+                         PyObject *current_item_separator)


current_newline_indent and current_item_separator are pretty long names. What if remove the current_ prefix from them?

Wouldn't we need new_newline_indent then?

Removing the prefix works for this method, but in the other methods that take the same argument there already was a newline_indent. I took nineteendo's suggestion to rename those to new_newline_indent

Modules/_json.c

serhiy-storchaka

LGTM. Please add a NEWS entry, and it will be ready for merge.

Your initial idea about caching looks interesting. It does not even need a dict, it can use a list. And the implementation may be not more complex than this PR, maybe even simpler.

Modules/_json.c

serhiy-storchaka · 2024-05-03T15:15:10Z

I have implemented caching. It is not simpler than the current PR, so I'll open a separate PR after merging this PR.

Rewrite _creat_newline_indent using PyUnicode_AppendAndDel Co-authored-by: Serhiy Storchaka <[email protected]>

Misc/NEWS.d/next/Core and Builtins/2024-05-03-18-01-26.gh-issue-95382.73FSEv.rst

…e-95382.73FSEv.rst Co-authored-by: Jelle Zijlstra <[email protected]>

serhiy-storchaka · 2024-05-06T08:41:59Z

Thank you for your contribution @eendebakpt. The main advantage of this PR is that it is simple, so while serialization with indentations is not used in performance critical code (because indentation increases the volume of the output, and therefore makes it slower), it is nice to have such speed up almost for free.

eendebakpt · 2024-05-06T09:08:27Z

Nice to have this in just before the beta. Thanks to all the reviewers for suggestions and refleak hunting skills!

…onGH-118105)

eendebakpt added 5 commits April 18, 2024 20:44

Fix docstring of _json.Encoder

c1cfcf5

Implement indent for _json

30f2e72

refactor

1da39f3

update comments

0d85551

fix decrefs

cc02a13

bedevere-app bot mentioned this pull request Apr 19, 2024

Remove unused indent_level from Modules/_json.c #95382

Closed

bedevere-app bot added the awaiting review label Apr 19, 2024

corona10 added the skip news label Apr 19, 2024

corona10 requested a review from serhiy-storchaka April 19, 2024 15:32

This comment was marked as resolved.

Sign in to view

eendebakpt changed the title ~~Draft: gh-95382: Improve performance of json encoder with indent~~ gh-95382: Improve performance of json encoder with indent Apr 19, 2024

nineteendo reviewed Apr 20, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

eendebakpt commented Apr 20, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

eendebakpt and others added 2 commits April 20, 2024 08:58

Apply suggestions from code review

a444701

Co-authored-by: Nice Zombies <[email protected]>

Merge branch 'main' into json_c_indent

8d4d48a

JelleZijlstra reviewed Apr 21, 2024

View reviewed changes

eendebakpt added 2 commits April 21, 2024 20:58

review comments

ed989b8

whitespace

5df567b

nineteendo reviewed Apr 21, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

eendebakpt added 2 commits April 21, 2024 21:46

whitespace

5fdc279

Merge branch 'main' into json_c_indent

1ec550f

JelleZijlstra reviewed Apr 21, 2024

View reviewed changes

serhiy-storchaka reviewed Apr 22, 2024

View reviewed changes

address review comments

5e47a41

nineteendo reviewed Apr 24, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

review comments

a407b84

nineteendo reviewed Apr 25, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

Update Modules/_json.c

ac86ee4

Co-authored-by: Nice Zombies <[email protected]>

serhiy-storchaka reviewed Apr 25, 2024

View reviewed changes

eendebakpt added 4 commits April 25, 2024 21:07

review comments

3b55d64

rename newline_indent variables

bb4ff43

code style

9ef9332

rename variable

ed029a6

nineteendo reviewed Apr 25, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

eendebakpt requested a review from serhiy-storchaka May 3, 2024 09:44

serhiy-storchaka reviewed May 3, 2024

View reviewed changes

Modules/_json.c Outdated Show resolved Hide resolved

eendebakpt and others added 2 commits May 3, 2024 17:18

Update Modules/_json.c

36e3313

Rewrite _creat_newline_indent using PyUnicode_AppendAndDel Co-authored-by: Serhiy Storchaka <[email protected]>

Merge branch 'main' into json_c_indent

b69d08e

serhiy-storchaka removed the skip news label May 3, 2024

📜🤖 Added by blurb_it.

e78ff6a

JelleZijlstra reviewed May 3, 2024

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2024-05-03-18-01-26.gh-issue-95382.73FSEv.rst Outdated Show resolved Hide resolved

eendebakpt and others added 3 commits May 3, 2024 20:55

Update Misc/NEWS.d/next/Core and Builtins/2024-05-03-18-01-26.gh-issu…

5c40126

…e-95382.73FSEv.rst Co-authored-by: Jelle Zijlstra <[email protected]>

update news entry

a43f2f2

Merge branch 'main' into json_c_indent

f2b0c06

serhiy-storchaka merged commit 05adfbb into python:main May 6, 2024
36 checks passed

bedevere-app bot removed the awaiting review label May 6, 2024

serhiy-storchaka mentioned this pull request May 6, 2024

gh-95382: Use cache for indentations in the JSON encoder #118636

Merged

nineteendo mentioned this pull request May 6, 2024

gh-118299: Re-use old indent in json.loads() #118300

Closed

SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024

pythongh-95382: Improve performance of json encoder with indent (pyth…

562eb8b

…onGH-118105)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-95382: Improve performance of json encoder with indent #118105

gh-95382: Improve performance of json encoder with indent #118105

eendebakpt commented Apr 19, 2024 •

edited

Loading

This comment was marked as resolved.

JelleZijlstra Apr 21, 2024

JelleZijlstra Apr 21, 2024

eendebakpt Apr 21, 2024

JelleZijlstra Apr 21, 2024

eendebakpt Apr 21, 2024

nineteendo commented Apr 21, 2024

nineteendo commented Apr 21, 2024

nineteendo commented Apr 21, 2024

eendebakpt commented Apr 21, 2024

nineteendo commented Apr 21, 2024 •

edited

Loading

eendebakpt commented Apr 21, 2024 •

edited

Loading

JelleZijlstra Apr 21, 2024

nineteendo Apr 21, 2024

serhiy-storchaka Apr 22, 2024

JelleZijlstra Apr 21, 2024

serhiy-storchaka Apr 22, 2024

serhiy-storchaka Apr 22, 2024

serhiy-storchaka Apr 22, 2024

serhiy-storchaka Apr 22, 2024

This comment was marked as duplicate.

nineteendo commented Apr 24, 2024 •

edited

Loading

serhiy-storchaka Apr 25, 2024

serhiy-storchaka Apr 25, 2024

nineteendo Apr 25, 2024

eendebakpt Apr 25, 2024

serhiy-storchaka left a comment

serhiy-storchaka commented May 3, 2024

serhiy-storchaka commented May 6, 2024

eendebakpt commented May 6, 2024

		PyObject *current_newline_indent,
		PyObject *current_item_separator)

gh-95382: Improve performance of json encoder with indent #118105

gh-95382: Improve performance of json encoder with indent #118105

Conversation

eendebakpt commented Apr 19, 2024 • edited Loading

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nineteendo commented Apr 21, 2024

nineteendo commented Apr 21, 2024

nineteendo commented Apr 21, 2024

eendebakpt commented Apr 21, 2024

nineteendo commented Apr 21, 2024 • edited Loading

eendebakpt commented Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

nineteendo commented Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented May 3, 2024

serhiy-storchaka commented May 6, 2024

eendebakpt commented May 6, 2024

eendebakpt commented Apr 19, 2024 •

edited

Loading

nineteendo commented Apr 21, 2024 •

edited

Loading

eendebakpt commented Apr 21, 2024 •

edited

Loading

nineteendo commented Apr 24, 2024 •

edited

Loading