Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serhiy-storchaka merged 30 commits into from
May 6, 2024
Merged

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

Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c1cfcf5
Fix docstring of _json.Encoder
eendebakpt Apr 18, 2024
30f2e72
Implement indent for _json
eendebakpt Apr 18, 2024
1da39f3
refactor
eendebakpt Apr 18, 2024
0d85551
update comments
eendebakpt Apr 19, 2024
cc02a13
fix decrefs
eendebakpt Apr 19, 2024
a444701
Apply suggestions from code review
eendebakpt Apr 20, 2024
8d4d48a
Merge branch 'main' into json_c_indent
JelleZijlstra Apr 21, 2024
ed989b8
review comments
eendebakpt Apr 21, 2024
5df567b
whitespace
eendebakpt Apr 21, 2024
5fdc279
whitespace
eendebakpt Apr 21, 2024
1ec550f
Merge branch 'main' into json_c_indent
eendebakpt Apr 21, 2024
5e47a41
address review comments
eendebakpt Apr 22, 2024
eefc508
Merge branch 'main' into json_c_indent
eendebakpt Apr 22, 2024
a7f4bc6
pep7
eendebakpt Apr 22, 2024
35601c7
pep7
eendebakpt Apr 22, 2024
311b7df
Update Modules/_json.c
eendebakpt Apr 24, 2024
ed2c806
Update Modules/_json.c
eendebakpt Apr 24, 2024
2faf554
Merge branch 'main' into json_c_indent
eendebakpt Apr 24, 2024
a407b84
review comments
eendebakpt Apr 25, 2024
ac86ee4
Update Modules/_json.c
eendebakpt Apr 25, 2024
3b55d64
review comments
eendebakpt Apr 25, 2024
bb4ff43
rename newline_indent variables
eendebakpt Apr 25, 2024
9ef9332
code style
eendebakpt Apr 25, 2024
ed029a6
rename variable
eendebakpt Apr 25, 2024
36e3313
Update Modules/_json.c
eendebakpt May 3, 2024
b69d08e
Merge branch 'main' into json_c_indent
eendebakpt May 3, 2024
e78ff6a
📜🤖 Added by blurb_it.
blurb-it[bot] May 3, 2024
5c40126
Update Misc/NEWS.d/next/Core and Builtins/2024-05-03-18-01-26.gh-issu…
eendebakpt May 3, 2024
a43f2f2
update news entry
eendebakpt May 3, 2024
f2b0c06
Merge branch 'main' into json_c_indent
eendebakpt May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
review comments
  • Loading branch information
@eendebakpt
eendebakpt committed Apr 25, 2024
commit a407b847285ab4c49ad9ccfd6a029ab3414f7382
42 changes: 27 additions & 15 deletions Modules/_json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,7 +1258,7 @@ _create_newline_indent(PyObject* indent, Py_ssize_t indent_level)
if (current_indent == NULL) {
return NULL;
}
PyObject* start = PyUnicode_FromOrdinal(10);
PyObject* start = PyUnicode_FromOrdinal('\n');
if (start == NULL) {
Py_DECREF(current_indent);
return NULL;
Expand Down Expand Up @@ -1288,13 +1288,15 @@ encoder_call(PyEncoderObject *self, PyObject *args, PyObject *kwds)

PyObject * current_newline_indent = NULL;
if (self->indent != Py_None) {
current_newline_indent = _create_newline_indent(self->indent, indent_level);
current_newline_indent = _create_newline_indent(self->indent,
indent_level);
if (current_newline_indent == NULL) {
_PyUnicodeWriter_Dealloc(&writer);
return NULL;
}
}
if (encoder_listencode_obj(self, &writer, obj, indent_level, current_newline_indent)) {
if (encoder_listencode_obj(self, &writer, obj, indent_level,
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

current_newline_indent)) {
_PyUnicodeWriter_Dealloc(&writer);
return NULL;
}
Expand Down Expand Up @@ -1384,10 +1386,10 @@ _steal_accumulate(_PyUnicodeWriter *writer, PyObject *stolen)
return rval;
}


static int
encoder_listencode_obj(PyEncoderObject *s, _PyUnicodeWriter *writer,
PyObject *obj, Py_ssize_t indent_level, PyObject *current_newline_indent)
PyObject *obj, Py_ssize_t indent_level,
PyObject *current_newline_indent)
{
/* Encode Python object obj to a JSON term */
PyObject *newobj;
Expand Down Expand Up @@ -1423,14 +1425,16 @@ encoder_listencode_obj(PyEncoderObject *s, _PyUnicodeWriter *writer,
else if (PyList_Check(obj) || PyTuple_Check(obj)) {
if (_Py_EnterRecursiveCall(" while encoding a JSON object"))
return -1;
rv = encoder_listencode_list(s, writer, obj, indent_level, current_newline_indent);
rv = encoder_listencode_list(s, writer, obj, indent_level,
current_newline_indent);
_Py_LeaveRecursiveCall();
return rv;
}
else if (PyDict_Check(obj)) {
if (_Py_EnterRecursiveCall(" while encoding a JSON object"))
return -1;
rv = encoder_listencode_dict(s, writer, obj, indent_level, current_newline_indent);
rv = encoder_listencode_dict(s, writer, obj, indent_level,
current_newline_indent);
_Py_LeaveRecursiveCall();
return rv;
}
Expand Down Expand Up @@ -1464,7 +1468,8 @@ encoder_listencode_obj(PyEncoderObject *s, _PyUnicodeWriter *writer,
Py_XDECREF(ident);
return -1;
}
rv = encoder_listencode_obj(s, writer, newobj, indent_level, current_newline_indent);
rv = encoder_listencode_obj(s, writer, newobj, indent_level,
current_newline_indent);
_Py_LeaveRecursiveCall();

Py_DECREF(newobj);
Expand All @@ -1486,7 +1491,8 @@ encoder_listencode_obj(PyEncoderObject *s, _PyUnicodeWriter *writer,
static int
encoder_encode_key_value(PyEncoderObject *s, _PyUnicodeWriter *writer, bool *first,
PyObject *key, PyObject *value, Py_ssize_t indent_level,
PyObject *current_newline_indent, PyObject *current_item_separator)
PyObject *current_newline_indent,
PyObject *current_item_separator)
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@nineteendo nineteendo Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we need new_newline_indent then?

Copy link
Copy Markdown
Contributor Author

@eendebakpt eendebakpt Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
PyObject *keystr = NULL;
PyObject *encoded;
Expand Down Expand Up @@ -1541,7 +1547,8 @@ encoder_encode_key_value(PyEncoderObject *s, _PyUnicodeWriter *writer, bool *fir
if (_PyUnicodeWriter_WriteStr(writer, s->key_separator) < 0) {
return -1;
}
if (encoder_listencode_obj(s, writer, value, indent_level, current_newline_indent) < 0) {
if (encoder_listencode_obj(s, writer, value, indent_level,
current_newline_indent) < 0) {
return -1;
}
return 0;
Expand Down Expand Up @@ -1593,7 +1600,8 @@ encoder_listencode_dict(PyEncoderObject *s, _PyUnicodeWriter *writer,
if (separator_indent == NULL) {
goto bail;
}
current_item_separator = separator_indent; // update item separator with a borrowed reference
// update item separator with a borrowed reference
current_item_separator = separator_indent;
if (_PyUnicodeWriter_WriteStr(writer, newline_indent) < 0) {
goto bail;
}
Expand All @@ -1614,15 +1622,19 @@ encoder_listencode_dict(PyEncoderObject *s, _PyUnicodeWriter *writer,

key = PyTuple_GET_ITEM(item, 0);
value = PyTuple_GET_ITEM(item, 1);
if (encoder_encode_key_value(s, writer, &first, key, value, indent_level, newline_indent, current_item_separator) < 0)
if (encoder_encode_key_value(s, writer, &first, key, value,
indent_level, newline_indent,
current_item_separator) < 0)
goto bail;
}
Py_CLEAR(items);

} else {
Py_ssize_t pos = 0;
while (PyDict_Next(dct, &pos, &key, &value)) {
if (encoder_encode_key_value(s, writer, &first, key, value, indent_level, newline_indent, current_item_separator) < 0)
if (encoder_encode_key_value(s, writer, &first, key, value,
indent_level, newline_indent,
current_item_separator) < 0)
goto bail;
}
}
Expand All @@ -1644,7 +1656,6 @@ encoder_listencode_dict(PyEncoderObject *s, _PyUnicodeWriter *writer,

if (_PyUnicodeWriter_WriteChar(writer, '}'))
goto bail;

return 0;

bail:
Expand All @@ -1657,7 +1668,8 @@ encoder_listencode_dict(PyEncoderObject *s, _PyUnicodeWriter *writer,

static int
encoder_listencode_list(PyEncoderObject *s, _PyUnicodeWriter *writer,
PyObject *seq, Py_ssize_t indent_level, PyObject *current_newline_indent)
PyObject *seq, Py_ssize_t indent_level,
PyObject *current_newline_indent)
{
PyObject *ident = NULL;
PyObject *s_fast = NULL;
Expand Down