8000 bpo-34100: compile: Re-enable frozenset merging by methane · Pull Request #10760 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-34100: compile: Re-enable frozenset merging #10760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 28, 2018
Merged

bpo-34100: compile: Re-enable frozenset merging #10760

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
77ca8b0
Revert "bpo-34100: Partially revert merge_consts_recursive() (GH-10743)"
methane Nov 28, 2018
d7fbb05
Fix reference leak
methane Nov 28, 2018
7a520e4
frozenset item is merged now
methane Nov 28, 2018
86d305e
Resurrect special casing empty frozenset
methane Nov 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
8000
Diff view
Prev Previous commit
Next Next commit
Fix reference leak
  • Loading branch information
@methane
methane committed Nov 28, 2018
commit d7fbb0581e7caacadf450e87d344559c17c0043b
24 changes: 13 additions & 11 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to do it backward?

  • First merge items
  • Then merge the container

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() :-(

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) {
Expand All @@ -1241,20 +1245,13 @@ 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.
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);
Expand All @@ -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);
}

Expand Down
0