extmod/modujson: Allow json.loads() to preserve dict order. #6135

andrewleech · 2020-06-10T03:20:26Z

When MICROPY_PY_UJSON_ORDERED_DICT is enabled.

Migrating away from using my previous #5323 PR to make all dict's ordered has unearthed a few other places we were relying on such functionality.

tve · 2020-06-11T04:26:49Z

extmod/modujson.c

@@ -224,6 +224,12 @@ STATIC mp_obj_t mod_ujson_load(mp_obj_t stream_obj) {
                break;
            case '{':
                next = mp_obj_new_dict(0);
+                #if MICROPY_PY_UJSON_ORDERED_DICT


Is this a new option? Is it set anywhere?

It is a new option, and no it's not set by default anywhere. The expectation is it'll be set in make option or board profile if someone wants it... or if it's a desieable enough feature it could be made the default

Hmm, doesn't that make it almost impossible to discover? Someone has to go look at the code to see "hey, look at that! I can enable MICROPY_PY_UJSON_ORDERED_DICT!". Just wondering...

Yeah it's certainly not ideal... At the very least I should enable it on the coverage build and add a unit test - this initial PR was thrown up very quickly in the middle of a project rebase/test/release cycle on my end and I didn't want it to be forgotten.

Just put in in mpconfig.h, like all other options.

andrewleech · 2020-06-14T07:46:39Z

Arguably, this could be good to have enabled by MICROPY_CPYTHON_COMPAT rather than a new define as cpython does already load json's in order thanks to their ordered by default dict's.

As there are performance differences between regular and ordered dicts still in micropython I'm happy to have it as a separate feature enable flag for now.

I've now added the flag in mpconfig.h and added/updated unit test in coverage build.

stinos · 2020-06-14T07:50:16Z

tests/extmod/ujson_loads.py


 def my_print(o):
    if isinstance(o, dict):
-        print("sorted dict", sorted(o.items()))
+        if is_coverage:  # Has ordered json loads enabled
+            print("dict", o.items())


Just to check, this assumes the CPython used to run the test will also preserve dict order. Do all CPython versions do this, and if not: from which version on? I.e. we whould be sure this always works.

Ah yes, that is the assumption yes. This has been the case since Python 3.6, though this aspect was written in as official in 3.7
https://docs.python.org/3.6/whatsnew/3.6.html#whatsnew36-compactdict

Arguably we should look at bringing in this same dict implementation as an option in micropython, it was shown to be significantly more efficient than the previous cpython dict and would likely be better than our current ordered dict implementation at least (certainly for very large dicts)

... and yes this is why the new unit test is failing, cpython 3.5 is used on travis :-(

Perhaps rather than making this a #define feature I should have just added the object_pairs_hook argument like in cpython:
https://docs.python.org/3.4/library/json.html#json.load

It would be more work to add this level of generic support rather than just added an ordered flag however

It would be more work to add this level of generic support rather than just added an ordered flag however

Would also be larger in code size.

Perhaps change the check so that when coverage is enabled, it just checks whether the returned dict is the same as sorted(dict).

Checking loaded dict == sorted dict results the same as the comparison I've already got in place - it works on micropython with my patch and cpython 3.6+ but does not work on cpython3.5. I can put an extra try block in there to loads with object_pairs_hook=OrderedDict for cpython though.

Ah but the idea is that CPython never runs any code for which it's behavior changes between versions, i.e. code guarded by is_coverage only gets executed by MicroPython, not CPython, and the else clause is where the expected result gets printed. Pseudocode:

if is_coverage: print(is_dict_with_same_content_and_sorted(a,b)) else: print(True)

Ah I didn't realise that's how it would work, that's brilliant, thanks. I've updated as such.

stinos · 2020-12-04T07:05:40Z

extmod/modujson.c

@@ -224,6 +224,12 @@ STATIC mp_obj_t mod_ujson_load(mp_obj_t stream_obj) {
                break;
            case '{':
                next = mp_obj_new_dict(0);
+                #if MICROPY_PY_UJSON_ORDERED_DICT
+                // keep the locals ordered


Same comment as for other commit (plus this isn't about locals here)

codecov-commenter · 2021-07-13T00:49:24Z

Codecov Report

Merging #6135 (dcd5ccc) into master (6430cd3) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6135      +/-   ##
==========================================
- Coverage   98.28%   98.28%   -0.01%     
==========================================
  Files         154      154              
  Lines       19988    19997       +9     
==========================================
+ Hits        19646    19654       +8     
- Misses        342      343       +1

Impacted Files	Coverage Δ
extmod/modujson.c	`97.57% <100.00%> (+0.02%)`	⬆️
py/smallint.c	`92.00% <0.00%> (-4.00%)`	⬇️
py/nativeglue.c	`94.44% <0.00%> (-1.12%)`	⬇️
py/vm.c	`99.03% <0.00%> (-0.01%)`	⬇️
py/emitnative.c	`99.26% <0.00%> (-0.01%)`	⬇️
py/obj.h	`100.00% <0.00%> (ø)`
py/modsys.c	`100.00% <0.00%> (ø)`
py/objenumerate.c	`100.00% <0.00%> (ø)`
py/objgenerator.c	`100.00% <0.00%> (ø)`
py/objgetitemiter.c	`100.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6430cd3...dcd5ccc. Read the comment docs.

When MICROPY_PY_UJSON_ORDERED_DICT is enabled.

merge hexky_s2 board

tve reviewed Jun 11, 2020

View reviewed changes

andrewleech force-pushed the ordered_json_loads branch from ef1bc9e to 8bf982d Compare June 14, 2020 07:46

stinos reviewed Jun 14, 2020

View reviewed changes

andrewleech force-pushed the ordered_json_loads branch 2 times, most recently from 2064a42 to 6be22fb Compare June 15, 2020 20:08

dpgeorge mentioned this pull request Jun 18, 2020

Supporting insertion-order preservation property of dicts (compat with Python 3.7+) #6170

Open

andrewleech force-pushed the ordered_json_loads branch from 6be22fb to 659d752 Compare November 4, 2020 03:42

andrewleech force-pushed the ordered_json_loads branch from 659d752 to a64f4bc Compare November 15, 2020 23:48

andrewleech force-pushed the ordered_json_loads branch from a64f4bc to 71f39fd Compare December 4, 2020 05:00

stinos reviewed Dec 4, 2020

View reviewed changes

andrewleech force-pushed the ordered_json_loads branch from 71f39fd to 7fbe7d4 Compare July 13, 2021 00:09

andrewleech force-pushed the ordered_json_loads branch 2 times, most recently from dcd5ccc to 0e61ba2 Compare July 26, 2021 02:24

extmod/modujson: Allow json.loads() to preserve dict order.

e6e1463

When MICROPY_PY_UJSON_ORDERED_DICT is enabled.

andrewleech force-pushed the ordered_json_loads branch from 0e61ba2 to e6e1463 Compare July 28, 2021 23:04

dpgeorge added the extmod Relates to extmod/ directory in source label Nov 30, 2021

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Mar 15, 2022

Merge pull request micropython#6135 from CircuitART/main

862210b

merge hexky_s2 board

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extmod/modujson: Allow json.loads() to preserve dict order. #6135

extmod/modujson: Allow json.loads() to preserve dict order. #6135

andrewleech commented Jun 10, 2020

tve Jun 11, 2020

andrewleech Jun 11, 2020

tve Jun 11, 2020

andrewleech Jun 11, 2020

stinos Jun 11, 2020

andrewleech commented Jun 14, 2020

stinos Jun 14, 2020

andrewleech Jun 14, 2020

andrewleech Jun 14, 2020

andrewleech Jun 14, 2020 •

edited

Loading

stinos Jun 14, 2020

andrewleech Jun 14, 2020

stinos Jun 15, 2020

andrewleech Jun 15, 2020

stinos Dec 4, 2020

codecov-commenter commented Jul 13, 2021 •

edited

Loading

extmod/modujson: Allow json.loads() to preserve dict order. #6135

Are you sure you want to change the base?

extmod/modujson: Allow json.loads() to preserve dict order. #6135

Conversation

andrewleech commented Jun 10, 2020

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

andrewleech commented Jun 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewleech Jun 14, 2020 • 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

codecov-commenter commented Jul 13, 2021 • edited Loading

Codecov Report

andrewleech Jun 14, 2020 •

edited

Loading

codecov-commenter commented Jul 13, 2021 •

edited

Loading