PYTHON-4179: Optimize JSON decoding performance by avoiding object_pairs_hook

[ilukyanchikov]

Optimized performance by removing object_pairs_hook call if we need the default conversion to dict behavior jira task
In object_hook, a dictionary is already passed, and calling object_pairs_hook for casting pairs to dict is redundant.
I validated my changes with test_json_util cases. pytest -v -s test/test_json_util.py
Compare performance with TestJson*Decoding cases(changes affect performance only in these cases) pytest -v -s test/performance/perf_test.py::TestJsonFlatDecoding test/performance/perf_test.py::TestJsonDeepDecoding test/performance/perf_test.py::TestJsonFullDecoding

Test Name	master	current branch
JsonFlatDecoding	96.18636555725531	117.50816018425552
JsonDeepDecoding	58.337081713027956	77.52136671585596
JsonFullDecoding	33.81260536995342	38.88821810950474

This fix only works with JSONOptions where document_class is dict, and I haven't found cases where something other than dict was used.

[ShaneHarvey]

Nice work this is great!

[ShaneHarvey]

Now that native dictionaries are always ordered (we only support Python>=3.7), I don't think we ever need to use object_pairs_hook. I wonder if JSON decoding with document_class=SON or document_class=OrderedDict is also faster using object_hook instead of object_pairs_hook?

[ShaneHarvey]

I tested this and see a comically large improvement. Before:

$ python -m timeit -s '
from bson import SON
from bson.json_util import dumps,loads,DEFAULT_JSON_OPTIONS
opts=DEFAULT_JSON_OPTIONS.with_options(document_class=SON)
doc={str(i): {"a": 1, "b": 2} for i in range(10000)}
json_doc=dumps(doc)
' 'loads(json_doc, json_options=opts)'
1 loop, best of 5: 364 msec per loop

After:

$ python -m timeit -s '
from bson import SON
from bson.json_util import dumps,loads,DEFAULT_JSON_OPTIONS
opts=DEFAULT_JSON_OPTIONS.with_options(document_class=SON)
doc={str(i): {"a": 1, "b": 2} for i in range(10000)}
json_doc=dumps(doc)
' 'loads(json_doc, json_options=opts)'
50 loops, best of 5: 4.82 msec per loop

[ShaneHarvey]

To be clear I'm suggesting:

json_options = kwargs.pop("json_options", DEFAULT_JSON_OPTIONS)
kwargs["object_hook"] = lambda obj: object_hook(obj, json_options)
return json.loads(s, *args, **kwargs)

[ilukyanchikov]

To be clear I'm suggesting:

json_options = kwargs.pop("json_options", DEFAULT_JSON_OPTIONS)
kwargs["object_hook"] = lambda obj: object_hook(obj, json_options)
return json.loads(s, *args, **kwargs)

It looks like such changes will break backward compatibility and such code will throw an exception.

python -c '
from bson.son import SON1 as SON
from bson.json_util import dumps,loads,DEFAULT_JSON_OPTIONS
opts=DEFAULT_JSON_OPTIONS.with_options(document_class=SON)
doc={"1":"2", "2":{"3":"4"}}
json_sting=dumps(doc)
obj=loads(json_sting, json_options=opts)
obj["2"].to_dict()
'
Traceback (most recent call last):
  File "<string>", line 8, in <module>
AttributeError: 'dict' object has no attribute 'to_dict'

If this is okay, then we can make this changes.

But if we want to maintain backward compatibility, I would suggest the following changes.

• Replace the SON implementation with such (It will speed up the code you mentioned in this comment PYTHON-4179: Optimize JSON decoding performance by avoiding object_pairs_hook #1493 (comment))

# The only reason for this class is to maintain backward compatibility
# so that the code son_obj.to_dict() and son_obj[key].to_dict() works correctly.
class SON(Dict[_Key, _Value]):
    def __init__(self, *args, **kwargs):
        warnings.warn(
            "Class SON is deprecated and will be removed in version x.x.x; use the default Python dict instead",
            category=DeprecationWarning, stacklevel=2)
        super().__init__(*args, **kwargs)
        for k, v in self.items():
            if isinstance(v, dict):
                self[k] = SON(v)

    def __repr__(self):
        return f"SON{super().__repr__()}"

    def to_dict(self) -> dict[_Key, _Value]:
        ...

• Add similar warning if we pass/set the document_class in the JSONOptions object.

And in version x.x.x we can change loads function behavior and remove document_class, and also possibly get rid of SON.

[ilukyanchikov]

python -m timeit -s '
from bson.son import SON_OLD as SON
from bson.json_util import dumps,loads,DEFAULT_JSON_OPTIONS
opts=DEFAULT_JSON_OPTIONS.with_options(document_class=SON)
doc={str(i): {"a": 1, "b": 2} for i in range(10000)}
json_doc=dumps(doc)
' 'loads(json_doc, json_options=opts)'
1 loop, best of 5: 427 msec per loop

python -m timeit -s '
from bson.son import SON as SON
from bson.json_util import dumps,loads,DEFAULT_JSON_OPTIONS
opts=DEFAULT_JSON_OPTIONS.with_options(document_class=SON)
doc={str(i): {"a": 1, "b": 2} for i in range(10000)}
json_doc=dumps(doc)
' 'loads(json_doc, json_options=opts)'
10 loops, best of 5: 25.7 msec per loop

[ShaneHarvey]

Oh I see. I was under the incorrect assumption that object_hook also casted the input to document_class.

[ShaneHarvey]

Also feel free to add your name to doc/contributors.rst if you like.

[NoahStapp]

As Shane said, great work!

[NoahStapp]

Suggested change

	if json_options.document_class is dict:
	if isinstance(json_options.document_class, dict):

Using isinstance supports inheritance, is dict only works for a literal dict object.

[ShaneHarvey]

This code will go away with my suggestion to always use object_hook above.

[ilukyanchikov]

I suppose what was meant here is issubclass(json_options.document_class, dict) because

>>> isinstance(dict, dict)
False

However, if you simply use issubclass, loads will return an object of the incorrect type, specifically always just dict.
Similar problem will occur if object_pairs_hook is completely removed without additional modifications. But I think it's worth trying to reduce the number of calls to the constructor of the dict-like class, which could optimize the execution time.

[ilukyanchikov]

Additionally, I've noticed that the behavior I described isn't being tested.
https://github.com/mongodb/mongo-python-driver/blob/master/test/test_json_util.py#L565 This test looks as though it's testing, but in reality, it doesn't check the type, and such a test
self.assertEqual(
SON([("foo", "bar"), ("b", 1)]),
{"foo": "bar", "b": 1}
)
will run without errors.

[ShaneHarvey]

Good catch. We need to add assertions that top-level and embedded objects are all SON with document_class=SON.

[ShaneHarvey]

Update: I'm added this test here: #1509

[ShaneHarvey]

@ilukyanchikov thanks for the great work here!

[ShaneHarvey]

The perf benchmarks confirm a 20-30% decoding improvement:

https://spruce.mongodb.com/task/mongo_python_driver_perf_tests_perf_6.0_standalone_97b9a333c84af5093874880d1d3ae7d5e59a8b59_24_02_05_21_59_14/trend-charts?execution=0