GH-137821: Convert `_json` to use Argument Clinic by hnnynh · Pull Request #138869 · python/cpython · GitHub

I will plan to follow up her PR during the core sprint.

Hmm, Would you like to explain why you remove xxxx_lock_held functions?
It's implemented for thread-safy in free-threading.

I didn't delete it myself; it got deleted when I ran make clinic.
Is it wrong to modify the code and then run make clinic?

I didn't delete it myself; it got deleted when I ran make clinic.
Is it wrong to modify the code and then run make clinic?

You should run make clinic first and then fill the declaration :)
But in that case, I think you should revert some change.

ditto

About benchmark, you have to install pyperf with your newly builded CPython

$> ./python.exe -m venv .venv
$> source .venv/bin/activate
$> pip install pyperf

And then write benchmark in this way (This is just example, I expect more possible data you can expect)

import pyperf
import _json


small_obj = '{"foo": "bar"}'

def bench_encode_basestring_ascii(s):
    return _json.encode_basestring_ascii(s)


runner = pyperf.Runner()
runner.bench_func("bench_encode_basestring_ascii_small", bench_encode_basestring_ascii, small_obj)

Finally you can run benchmark with following way.

$> python bench_json.py -o base.json (with main branch)
$>  python bench_json.py -o ac.json (with your working branch)
$>   pyperf compare_to base.json ac.json (will show your benchmark comparation)

About benchmark, you have to install pyperf with your newly builded CPython
...

I received the following execution results.

Benchmark hidden because not significant (1)

Since the code logic hasn't changed, I think the benchmark results should be the same and the result below is correct. Is that right?

Since the code logic hasn't changed, I think the benchmark results should be the same and the result below is correct. Is that right?

We suspsect the possibiity of overhead that are generated by AC but it looks not. But you also need to to write benchmark code for scanstring and encode_basestring.
And also test large size input for encode_basestring_ascii too

You can use string as pystr to preserve original parameter name in Python. Thought, it's a positional-only argument and it's less important.

Wait.

This paragraph of docstring is entirely lost. I think you shouldn't alter docstrings, except for case where you need to split out "summary line", per PEP 257. Everything else should go to a separate pr.

Again, you can rename generated function to keep old name:

/*[clinic input]
_json.encode_basestring_ascii as py_encode_basestring_ascii
...

Why is this? I think it should be reverted, just as _encoder_iterate_fast_seq_lock_held() removal below.

@skirpichev I am now mentoring her, so I will ping you once she is ready to get review :) It's not ready now.

Convert _json

1dc504d

This comment was marked as resolved.

Sign in to view

bedevere-app Bot mentioned this pull request Sep 13, 2025

Convert _json to Argument Clinic #137821

Closed

StanFromIreland added the skip news label Sep 13, 2025

corona10 self-assigned this Sep 13, 2025

corona10 reviewed Sep 14, 2025

View reviewed changes

Comment thread Modules/_json.c

corona10 reviewed Sep 14, 2025

View reviewed changes

fix: pep7 code structure

84d9e5e

skirpichev reviewed Sep 15, 2025

View reviewed changes

Revert changes up to 1dc504d

132218a

hnnynh closed this Oct 30, 2025

-              );
+              /*[clinic input]
+              _json.scanstring
+                  pystr: object

-                  "character in s after the quote that started the JSON string.\n"
-                  "Unescapes all valid JSON string escape sequences and raises ValueError\n"
-                  "on attempt to decode an invalid string. If strict is False then literal\n"
-                  "control characters are allowed in the string.\n"

                   if (PyCFunction_Check(s->encoder)) {
                       PyCFunction f = PyCFunction_GetFunction(s->encoder);
-                      if (f == py_encode_basestring_ascii) {
+                      if (f == _json_encode_basestring_ascii) {

               }
-              static inline int
-              _encoder_iterate_mapping_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer,

		ident = NULL;
		s_fast = PySequence_Fast(seq, "_iterencode_list needs a sequence");

GH-137821: Convert `_json` to use Argument Clinic by hnnynh · Pull Request #138869 · python/cpython · GitHub

Uh oh!

Uh oh!

GH-137821: Convert _json to use Argument Clinic - #138869

GH-137821: Convert _json to use Argument Clinic#138869hnnynh wants to merge 3 commits intopython:mainpython/cpython:mainfrom hnnynh:gh-137821hnnynh/cpython:gh-137821Copy head branch name to clipboard

Conversation

hnnynh commented Sep 13, 2025

Uh oh!

This comment was marked as resolved.

corona10 commented Sep 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnnynh commented Sep 14, 2025

Uh oh!

corona10 commented Sep 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 commented Sep 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-137821: Convert _json to use Argument Clinic#138869
hnnynh wants to merge 3 commits into
python:mainfrom
hnnynh:gh-137821

corona10 left a comment •
edited
Loading