From 77ca8b09d827953ac2dd4f6c4de2a2be01c49408 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Wed, 28 Nov 2018 18:35:45 +0900 Subject: [PATCH 1/4] Revert "bpo-34100: Partially revert merge_consts_recursive() (GH-10743)" This reverts commit 1005c84535191a72ebb7587d8c5636a065b7ed79. --- Lib/test/test_compile.py | 10 ++++++++ Python/compile.c | 50 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index a086ef65b44a6c..58bd9b5e4c7a08 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -615,6 +615,16 @@ def check_same_constant(const): self.check_constant(f1, Ellipsis) self.assertEqual(repr(f1()), repr(Ellipsis)) + # Merge constants in tuple or frozenset + # NOTE: frozenset can't reuse previous const, but frozenset + # item can be reused later. + f3 = lambda x: x in {("not a name",)} + f1, f2 = lambda: "not a name", lambda: ("not a name",) + self.assertIs(next(iter(f3.__code__.co_consts[1])), + f2.__code__.co_consts[1]) + self.assertIs(f1.__code__.co_consts[1], + f2.__code__.co_consts[1][0]) + # {0} is converted to a constant frozenset({0}) by the peephole # optimizer f1, f2 = lambda x: x in {0}, lambda x: x in {0} diff --git a/Python/compile.c b/Python/compile.c index 7d51819e00f0d9..acb5cfe29b42b3 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -1240,6 +1240,56 @@ merge_consts_recursive(struct compiler *c, PyObject *o) Py_DECREF(u); } } + else if (PyFrozenSet_CheckExact(o)) { + // We register items in the frozenset, but don't rewrite + // the frozenset when the item is already registered + // because frozenset is rare and difficult. + + // *key* is tuple. And it's first item is frozenset of + // constant keys. + // See _PyCode_ConstantKey() for detail. + assert(PyTuple_CheckExact(key)); + assert(PyTuple_GET_SIZE(key) == 2); + + Py_ssize_t len = PySet_GET_SIZE(o); + if (len == 0) { + return key; + } + PyObject *tuple = PyTuple_New(len); + if (tuple == NULL) { + Py_DECREF(key); + return NULL; + } + Py_ssize_t i = 0, pos = 0; + PyObject *item; + Py_hash_t hash; + while (_PySet_NextEntry(o, &pos, &item, &hash)) { + PyObject *k = merge_consts_recursive(c, item); + if (k == NULL) { + Py_DECREF(tuple); + Py_DECREF(key); + return NULL; + } + PyObject *u; + if (PyTuple_CheckExact(k)) { + u = PyTuple_GET_ITEM(k, 1); + } + else { + u = k; + } + Py_INCREF(u); + PyTuple_SET_ITEM(tuple, i, u); + i++; + } + + PyObject *new = PyFrozenSet_New(tuple); + Py_DECREF(tuple); + if (new == NULL) { + Py_DECREF(key); + return NULL; + } + PyTuple_SET_ITEM(key, 1, new); + } return key; } From d7fbb0581e7caacadf450e87d344559c17c0043b Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Wed, 28 Nov 2018 19:09:36 +0900 Subject: [PATCH 2/4] Fix reference leak --- Python/compile.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/Python/compile.c b/Python/compile.c index acb5cfe29b42b3..fc115428c91495 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -1208,14 +1208,18 @@ merge_consts_recursive(struct compiler *c, PyObject *o) // t is borrowed reference PyObject *t = PyDict_SetDefault(c->c_const_cache, key, key); if (t != key) { + // o is registered in c_const_cache. Just use it. Py_INCREF(t); Py_DECREF(key); return t; } + // We registered o in c_const_cache. + // When o is a tuple or frozenset, we want to merge it's + // items too. if (PyTuple_CheckExact(o)) { - Py_ssize_t i, len = PyTuple_GET_SIZE(o); - for (i = 0; i < len; i++) { + Py_ssize_t len = PyTuple_GET_SIZE(o); + for (Py_ssize_t i = 0; i < len; i++) { PyObject *item = PyTuple_GET_ITEM(o, i); PyObject *u = merge_consts_recursive(c, item); if (u == NULL) { @@ -1241,10 +1245,6 @@ merge_consts_recursive(struct compiler *c, PyObject *o) } } else if (PyFrozenSet_CheckExact(o)) { - // We register items in the frozenset, but don't rewrite - // the frozenset when the item is already registered - // because frozenset is rare and difficult. - // *key* is tuple. And it's first item is frozenset of // constant keys. // See _PyCode_ConstantKey() for detail. @@ -1252,9 +1252,6 @@ merge_consts_recursive(struct compiler *c, PyObject *o) assert(PyTuple_GET_SIZE(key) == 2); Py_ssize_t len = PySet_GET_SIZE(o); - if (len == 0) { - return key; - } PyObject *tuple = PyTuple_New(len); if (tuple == NULL) { Py_DECREF(key); @@ -1273,21 +1270,26 @@ merge_consts_recursive(struct compiler *c, PyObject *o) PyObject *u; if (PyTuple_CheckExact(k)) { u = PyTuple_GET_ITEM(k, 1); + Py_INCREF(u); + Py_DECREF(k); } else { u = k; } - Py_INCREF(u); - PyTuple_SET_ITEM(tuple, i, u); + PyTuple_SET_ITEM(tuple, i, u); // Steals reference of u. i++; } + // Instead of rewriting o, we create new frozenset and embed in the + // key tuple. Caller should get merged frozenset from the key tuple. PyObject *new = PyFrozenSet_New(tuple); Py_DECREF(tuple); if (new == NULL) { Py_DECREF(key); return NULL; } + assert(PyTuple_GET_ITEM(key, 1) == o); + Py_DECREF(o); PyTuple_SET_ITEM(key, 1, new); } From 7a520e4f9b5fd05de8427e704842860078a701fe Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Wed, 28 Nov 2018 19:15:20 +0900 Subject: [PATCH 3/4] frozenset item is merged now --- Lib/test/test_compile.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index 58bd9b5e4c7a08..56f73f63153434 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -616,14 +616,12 @@ def check_same_constant(const): self.assertEqual(repr(f1()), repr(Ellipsis)) # Merge constants in tuple or frozenset - # NOTE: frozenset can't reuse previous const, but frozenset - # item can be reused later. - f3 = lambda x: x in {("not a name",)} f1, f2 = lambda: "not a name", lambda: ("not a name",) - self.assertIs(next(iter(f3.__code__.co_consts[1])), - f2.__code__.co_consts[1]) + f3 = lambda x: x in {("not a name",)} self.assertIs(f1.__code__.co_consts[1], f2.__code__.co_consts[1][0]) + self.assertIs(next(iter(f3.__code__.co_consts[1])), + f2.__code__.co_consts[1]) # {0} is converted to a constant frozenset({0}) by the peephole # optimizer From 86d305e08ce78024cc487404c35e2b4748b98be6 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Wed, 28 Nov 2018 20:52:12 +0900 Subject: [PATCH 4/4] Resurrect special casing empty frozenset --- Python/compile.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Python/compile.c b/Python/compile.c index fc115428c91495..45e78cb22cd824 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -1252,6 +1252,9 @@ merge_consts_recursive(struct compiler *c, PyObject *o) assert(PyTuple_GET_SIZE(key) == 2); Py_ssize_t len = PySet_GET_SIZE(o); + if (len == 0) { // empty frozenset should not be re-created. + return key; + } PyObject *tuple = PyTuple_New(len); if (tuple == NULL) { Py_DECREF(key);