bpo-34100: compile: Re-enable frozenset merging

[methane]

https://bugs.python.org/issue34100

[vstinner]

Would it be possible to do it backward?

• First merge items
• Then merge the container

[methane]

I think it just make program more complicated. We need to check the container is not registered yet before merging items. So we need to generate key tuple containing the container object first anyway.

[vstinner]

Would it be possible to add the newly merged container to the cache rather than modifying the existing cache entry in-place?

[methane]

It's possible. This commit rewrites tuple merging without in-place modify of tuple and key.
2eb1471

But I'm not sure avoiding in-place edit is so important...
We have similar code already.
https://github.com/python/cpython/blob/master/Objects/codeobject.c#L47-L95

[vstinner]

https://github.com/python/cpython/blob/master/Objects/codeobject.c#L47-L95

Oh wow, this code is even worse: it modify the "immutable" arguments of PyCode_New() :-(

[vstinner]

I wrote an implementation of my proposal (merge items, then the container): PR #10762.

[methane]

I wrote an implementation of my proposal (merge items, then the container): PR #10762.

When there are 100 same 5-tuples, my PR makes key objects 100+5 times. In your PR, key object
is created 100*5 times.

[vstinner]

Travis CI is unhappy:

======================================================================
FAIL: test_singletons (test.test_ast.ConstantTests) (const=frozenset())
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_ast.py", line 1164, in test_singletons
    self.assertIs(value, const)
AssertionError: frozenset() is not frozenset()

[vstinner]

When there are 100 same 5-tuples, my PR makes key objects 100+5 times. In your PR, key object is created 100*5 times.

I wrote PR #10762 for the correctness more than for performance :-) Is it possible to measure the overhead of merge_consts_recursive() with a benchmark?

cc @serhiy-storchaka

[methane]

I wrote PR #10762 for the correctness more than for performance :-) Is it possible to measure the overhead of merge_consts_recursive() with a benchmark?

$ ./python.victor -m perf timeit --compare-to ./python.master 'compile("[" + "((1., 2.), (3., 4., 5.)),"*100 + "]", "", "exec")'
python.master: ..................... 770 us +- 34 us
python.victor: ..................... 857 us +- 35 us

Mean +- std dev: [python.master] 770 us +- 34 us -> [python.victor] 857 us +- 35 us: 1.11x slower (+11%)

Performance difference is relatively small because parser is slower than compiler.

[vstinner]

Oh thanks for the benchmark. Would you mind to run the benchmark on your PR as well?

[methane]

Oh thanks for the benchmark. Would you mind to run the benchmark on your PR as well?

This benchmark measures only tuples and floats. So my PR (only affects frozenset) doesn't have overhead.

$  ./python -m perf timeit --compare-to ./python.master 'compile("[" + "((1., 2.), (3., 4., 5.)),"*100 + "]", "", "exec")'
python.master: ..................... 772 us +- 35 us
python: ..................... 763 us +- 37 us

Mean +- std dev: [python.master] 772 us +- 35 us -> [python] 763 us +- 37 us: 1.01x faster (-1%)
Not significant!

[methane]

And when I removed merge_const_recursive:

$ ./python -m perf timeit --compare-to ./python.master 'compile("[" + "((1., 2.), (3., 4., 5.)),"*100 + "]", "", "exec")'
python.master: ..................... 774 us +- 37 us
python: ..................... 744 us +- 36 us

Mean +- std dev: [python.master] 774 us +- 37 us -> [python] 744 us +- 36 us: 1.04x faster (-4%)

$ git diff
diff --git a/Python/compile.c b/Python/compile.c
index 45e78cb22c..5f6247e9a1 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -1302,7 +1302,8 @@ merge_consts_recursive(struct compiler *c, PyObject *o)
 static Py_ssize_t
 compiler_add_const(struct compiler *c, PyObject *o)
 {
-    PyObject *key = merge_consts_recursive(c, o);
+    //PyObject *key = merge_consts_recursive(c, o);
+    PyObject *key = _PyCode_ConstantKey(o);
     if (key == NULL) {
         return -1;
     }

[vstinner]

LGTM. "./python -m test test_compile -R 3:3" pass.

I have no strong preference between your PR and my PR, as soon as the tests pass :-)