gh-78214: marshal: Stabilize FLAG_REF usage

[methane]

marshal.dumps() tests refcnt(obj)==1 to decide use FLAG_REF or not.

But refcnt of interned string is very unstable.
When compiling same source, refcnt of interned string in the output
may be 1 or >1. It makes FLAG_REF usage unstable.

To help reproducible build, use FLAG_REF for interned string even if
refcnt(obj)==1.

https://bugs.python.org/issue34033

[serhiy-storchaka]

I'm in process of reviewing.

[serhiy-storchaka]

Do you mind to provide also your initial PR? I want to try to optimize or simplify it.

[serhiy-storchaka]

Isn't using FLAG_REF for all interned strings slows down unmarshalling and increases the memory consumption for both marshalling and unmarshalling?

[methane]

Isn't using FLAG_REF for all interned strings slows down unmarshalling and increases the memory consumption for both marshalling and unmarshalling?

For unmarshaling speed, it's possible. But interned string has cost of interning; create temoporary string and calling PyDict_SetDefault(). Overhead of FLAG_REF (PyList_Append) is much smaller than it.

For marshaling memory overhead, it will increase hashtable size for each interned string with refcnt==1.
For unmarshaling memory overhead, it will increase list length too.

I don't expect it can be pragmatic problem though.

[mcepl]

@bmwiedemann Do you know about this one?

[bmwiedemann]

I might have seen this patch 2 years ago. Chances are, I didnt try it, because it did not apply cleanly to the python we had in Tumbleweed.

[mcepl]

@bmwiedemann Yes, you are right, this is basically non-applicable (especially the monstrosity in these two conflicting importlib*.h files).

@methane Do you think, you would be willing and able to port this PR to something more recent (e.g., 3.8 or master), please?

[methane]

importlib.h and importlib_external.h are generated files. You can apply 6c8ea7c cleanly and "make regen-importlib".

[mcepl]

So, @bmwiedemann what do you think about https://build.opensuse.org/package/show/home:mcepl:branches:devel:languages:python:Factory/python38 ?

[bmwiedemann]

@mcepl I tried my multibuildrbk script for a python38-base build there and got:

--- old /usr/lib64/python3.8/test/test_json/__pycache__/test_scanstring.cpython-38.pyc (hex)
+++ new /usr/lib64/python3.8/test/test_json/__pycache__/test_scanstring.cpython-38.pyc (hex)
@@ -1,6 +1,6 @@
 000002c0  00 00 54 29 02 f5 06 00  00 00 7a f0 9d 84 a0 78  |..T)......z....x|
 000002d0  e9 05 00 00 00 7a 08 22  5c 75 30 30 37 62 22 29  |.....z."\u007b")|
-000002e0  02 fa 01 7b e9 08 00 00  00 7a 3c 22 41 20 4a 53  |...{.....z<"A JS|
+000002e0  02 da 01 7b e9 08 00 00  00 7a 3c 22 41 20 4a 53  |...{.....z<"A JS|
 000002f0  4f 4e 20 70 61 79 6c 6f  61 64 20 73 68 6f 75 6c  |ON payload shoul|
 00000300  64 20 62 65 20 61 6e 20  6f 62 6a 65 63 74 20 6f  |d be an object o|
 00000310  72 20 61 72 72 61 79 2c  20 6e 6f 74 20 61 20 73  |r array, not a s|

--- old /usr/lib64/python3.8/__pycache__/netrc.cpython-38.pyc (hex)
+++ new /usr/lib64/python3.8/__pycache__/netrc.cpython-38.pyc (hex)
@@ -1,7 +1,7 @@
 000007c0  63 64 65 66 7a 02 20 09  da 01 0a 7a 04 20 09 0d  |cdefz. ....z. ..|
 000007d0  0a 7a 15 62 61 64 20 74  6f 70 6c 65 76 65 6c 20  |.z.bad toplevel |
 000007e0  74 6f 6b 65 6e 20 25 72  3e 04 00 00 00 72 1e 00  |token %r>....r..|
-000007f0  00 00 72 21 00 00 00 72  20 00 00 00 72 22 00 00  |..r!...r ...r"..|
+000007f0  00 00 72 22 00 00 00 72  20 00 00 00 72 21 00 00  |..r"...r ...r!..|
 00000800  00 7a 26 6d 61 6c 66 6f  72 6d 65 64 20 25 73 20  |.z&malformed %s |
 00000810  65 6e 74 72 79 20 25 73  20 74 65 72 6d 69 6e 61  |entry %s termina|
 00000820  74 65 64 20 62 79 20 25  73 da 05 6c 6f 67 69 6e  |ted by %s..login|

--- old /usr/lib64/python3.8/__pycache__/pathlib.cpython-38.pyc (hex)
+++ new /usr/lib64/python3.8/__pycache__/pathlib.cpython-38.pyc (hex)
@@ -1,5 +1,5 @@
 000079c0  64 20 27 2e 2e 27 2e 0a  20 20 20 20 20 20 20 20  |d '..'..        |
-000079d0  3e 02 00 00 00 72 32 00  00 00 72 a9 00 00 00 4e  |>....r2...r....N|
+000079d0  3e 02 00 00 00 72 a9 00  00 00 72 32 00 00 00 4e  |>....r....r2...N|
 000079e0  29 05 72 64 01 00 00 72  69 01 00 00 72 b5 00 00  |).rd...ri...r...|
 000079f0  00 72 d5 00 00 00 72 f7  00 00 00 72 44 01 00 00  |.r....r....rD...|
 00007a00  72 22 00 00 00 72 22 00  00 00 72 23 00 00 00 da  |r"...r"...r#....|

The last 2 could be a different issue of ordering.

[ericsnowcurrently]

We started freezing a bunch of stdlib modules last year, which definitely impacts the story here.

Regardless, treating all interned strings as "ref" objects makes sense, so +1 from me.

[ericsnowcurrently]

LGTM

[methane]

This is temporal fix and will be reverted after more complete fix is landed.