[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

STINNER Victor added the comment: > New changeset 3f4c319a822f by Serhiy Storchaka in branch '3.5': > Issue #24483: C implementation of functools.lru_cache() now calculates key's > https://hg.python.org/cpython/rev/3f4c319a822f I didn't follow this issue, but I like the change. Good job :)

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Changes by Serhiy Storchaka : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Roundup Robot added the comment: New changeset 3f4c319a822f by Serhiy Storchaka in branch '3.5': Issue #24483: C implementation of functools.lru_cache() now calculates key's https://hg.python.org/cpython/rev/3f4c319a822f New changeset 5758b85627c9 by Serhiy Storchaka in branch 'default': Issue

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

STINNER Victor added the comment: clru_cache_known_hash_5.larry.patch looks good to me. Python 3.5 has no more restriction to push patches, go ahead Serhiy, push it to 3.5 & 3.6. -- nosy: +haypo ___ Python tracker

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: Ping. -- ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Tal Einat added the comment: Ping? Let's not miss the final 3.5 beta. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483 ___ ___

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: Can you allow me to commit the patch or will commit it yourself Raymond? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483 ___

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Larry Hastings added the comment: This can go in for 3.5 beta 3. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483 ___ ___ Python-bugs-list

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Tal Einat added the comment: With clru_cache_known_hash_4.patch on the current default branch, the entire test suite passes here (OSX 10.10). -- nosy: +taleinat ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Meador Inge added the comment: I did some regression testing and reviewed the code; LGTM. As for the code structure issues, I agree that the duplication is undesirable (the readability argument is not that convincing), but Serhiy's patch is consistent with the existing design. As such, I

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: I would prefer do not repeat the code, but I afraid this can affect the performance, and the performance of PyDict_GetItem is critical for Python. -- ___ Python tracker rep...@bugs.python.org

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Raymond Hettinger added the comment: Please let this go forward as Serhiy has written it. That is the way I wrote the existing known hash function. You all should be focused on correctness, not on bike-shedding my and Serhiy's coding style. --

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Yury Selivanov added the comment: I would prefer do not repeat the code, but I afraid this can affect the performance, and the performance of PyDict_GetItem is critical for Python. Static function calls like that one will most likely be inlined by the compiler... But if you don't want to

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: But if you don't want to rely on that, maybe we can use a macro to avoid code duplication? This will not make the code cleaner. In any case this is other issue. -- title: Avoid repeated hash calculation in C implementation of

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Nick Coghlan added the comment: +1 to what Raymond says here. Concerns regarding code duplication need to be weighed against the likelihood of that code changing in the future, and the impact on the readability of each copy of the code in isolation. In this particular case, I think the

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Raymond Hettinger added the comment: Sorry Larry, but I prefer Serhiy's version. The known_hash variants need to have their own function intact version and gum up the rest of the code. See the current known_hash function for comparison (Serhiy modeled on what we had already decided to do

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Larry Hastings added the comment: I suggest that 20 lines of identical code copy-and-pasted between two functions is not the cleanest way to do it. Find attached my version of the patch, which splits this common code out into a static function. -- Added file:

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Larry Hastings added the comment: Patch doesn't build for me against current trunk: gcc -pthread -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes-Werror=declaration-after-statement -I. -IInclude -I./Include-DPy_BUILD_CORE -c

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: My bad. I submitted the last patch without checking (rebuilding Python takes too much time on my slow netbook). Here is fixed and tested patch. -- Added file: http://bugs.python.org/file39773/clru_cache_known_hash_4.patch

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Larry Hastings added the comment: I can accept this change, but I don't like that code. Is it really considered acceptable to have that much copy-and-paste code in the dict implementation for KnownHash calls? Could the common code be split off into a Py_LOCAL_INLINE function? --

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Raymond Hettinger added the comment: Serhiy's code looks like the cleanest way to do it. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483 ___

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

New submission from Serhiy Storchaka: There is a difference between Python and C implementations of functools.lru_cache(). Python implementation caches the hash of the key, C implementation doesn't. May be this is not too important (at least I have no an example that shows the benefit of

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: New patch adds _PyDict_DelItem_KnownHash() and uses it to guarantee that the hash is only calculated once. -- Added file: http://bugs.python.org/file39761/clru_cache_known_hash_2.patch ___ Python tracker

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Raymond Hettinger added the comment: This is important. -- assignee: - rhettinger ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483 ___ ___

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Raymond Hettinger added the comment: I would like the full functionality of the Python version to be implemented. Guaranteeing that the hash is only calculated once prevents a reentrancy hole and provides a speed benefit as well. Please implement exactly what the pure python version does

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Changes by Raymond Hettinger raymond.hettin...@gmail.com: -- Removed message: http://bugs.python.org/msg245600 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24483 ___

[issue24483] Avoid repeated hash calculation in C implementation of functools.lru_cache()

Serhiy Storchaka added the comment: New patch touches also unbounded cache version. Larry, do you allow to commit such patch in 3.5? -- nosy: +larry versions: +Python 3.5 Added file: http://bugs.python.org/file39766/clru_cache_known_hash_3.patch ___