Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(25155)

#14373: C implementation of functools.lru_cache

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 6 months ago by anacrolix
Modified:
4 years, 2 months ago
Reviewers:
storchaka, andrew.svetlov, pitrou, shadowranger+python
CC:
rhettinger, jcea, amaury.forgeotdarc, jaraco, AntoinePitrou, scoder, giampaolo.rodola, ned.deily, ezio.melotti, asvetlov, poke, meadori, Aaron.Meurer, BreamoreBoy, devnull_psf.upfronthosting.co.za, eric.snow, storchaka, brecht_mos6581.org, kmike84_gmail.com, kachayev, Josh.R
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Total comments: 21

Patch Set 5 #

Total comments: 19

Patch Set 6 #

Total comments: 6

Patch Set 7 #

Total comments: 13

Patch Set 8 #

Total comments: 8

Patch Set 9 #

Patch Set 10 #

Total comments: 41

Patch Set 11 #

Total comments: 8

Patch Set 12 #

Patch Set 13 #

Patch Set 14 #

Patch Set 15 #

Patch Set 16 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Modules/_functoolsmodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -5 lines 0 comments Download

Messages

Total messages: 19
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/4566/Lib/functools.py File Lib/functools.py (right): http://bugs.python.org/review/14373/diff/4566/Lib/functools.py#newcode14 Lib/functools.py:14: from _functools import partial, reduce, c_lru_cache Here is a ...
6 years, 9 months ago #1
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/6856/Lib/functools.py File Lib/functools.py (right): http://bugs.python.org/review/14373/diff/6856/Lib/functools.py#newcode233 Lib/functools.py:233: from _functools import c_lru_cache It would be better to ...
6 years, 9 months ago #2
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c#newcode742 Modules/_functoolsmodule.c:742: if (PyDict_DelItem(self->cache, link->key) < 0) { Py_DECREF(value); http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c#newcode763 Modules/_functoolsmodule.c:763: ...
6 years, 9 months ago #3
asvetlov
http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c#newcode742 Modules/_functoolsmodule.c:742: if (PyDict_DelItem(self->cache, link->key) < 0) { On 2012/12/21 22:22:09, ...
6 years, 9 months ago #4
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/6867/Modules/_functoolsmodule.c#newcode742 Modules/_functoolsmodule.c:742: if (PyDict_DelItem(self->cache, link->key) < 0) { On 2012/12/21 22:44:59, ...
6 years, 9 months ago #5
AntoinePitrou
A couple of comments. I haven't looked very deeply into this. http://bugs.python.org/review/14373/diff/6870/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): ...
6 years, 9 months ago #6
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/6870/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/6870/Modules/_functoolsmodule.c#newcode554 Modules/_functoolsmodule.c:554: } lru_list_elem; On 2012/12/22 21:49:49, AntoinePitrou wrote: > Why ...
6 years, 9 months ago #7
storchaka_gmail.com
6 years, 9 months ago #8
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/6870/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/6870/Modules/_functoolsmodule.c#newcode781 Modules/_functoolsmodule.c:781: Trailing whitespaces. http://bugs.python.org/review/14373/diff/6870/Modules/_functoolsmodule.c#newcode798 Modules/_functoolsmodule.c:798: maxsize = -1; Trailing whitespaces. ...
6 years, 9 months ago #9
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/6938/Lib/test/test_functools.py File Lib/test/test_functools.py (right): http://bugs.python.org/review/14373/diff/6938/Lib/test/test_functools.py#newcode829 Lib/test/test_functools.py:829: def test_lru_cache_threaded(self): The test should be skipped if threading ...
6 years, 8 months ago #10
AntoinePitrou
http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py File Lib/test/test_functools.py (right): http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py#newcode990 Lib/test/test_functools.py:990: @self.module.lru_cache(maxsize=-10) Is a negative size documented? Shouldn't it raise ...
5 years, 10 months ago #11
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py File Lib/test/test_functools.py (right): http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py#newcode990 Lib/test/test_functools.py:990: @self.module.lru_cache(maxsize=-10) On 2013/11/22 20:36:58, AntoinePitrou wrote: > Is a ...
5 years, 10 months ago #12
AntoinePitrou
http://bugs.python.org/review/14373/diff/10048/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/10048/Modules/_functoolsmodule.c#newcode796 Modules/_functoolsmodule.c:796: if (PyDict_DelItem(self->cache, link->key) < 0) { On 2013/11/22 21:54:56, ...
5 years, 10 months ago #13
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py File Lib/test/test_functools.py (right): http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py#newcode1084 Lib/test/test_functools.py:1084: # create 5 threads in order to fill cache ...
5 years, 10 months ago #14
AntoinePitrou
http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py File Lib/test/test_functools.py (right): http://bugs.python.org/review/14373/diff/10048/Lib/test/test_functools.py#newcode1110 Lib/test/test_functools.py:1110: On 2013/11/22 22:37:06, storchaka wrote: > On 2013/11/22 20:36:58, ...
5 years, 10 months ago #15
storchaka_gmail.com
http://bugs.python.org/review/14373/diff/10048/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/10048/Modules/_functoolsmodule.c#newcode796 Modules/_functoolsmodule.c:796: if (PyDict_DelItem(self->cache, link->key) < 0) { On 2013/11/22 22:09:29, ...
5 years, 10 months ago #16
Josh.R
Made some notes on possible improvements to threaded use case, and minor issues with argument ...
5 years, 5 months ago #17
storchaka_gmail.com
Thank you Josh for your comments. http://bugs.python.org/review/14373/diff/10053/Modules/_functoolsmodule.c File Modules/_functoolsmodule.c (right): http://bugs.python.org/review/14373/diff/10053/Modules/_functoolsmodule.c#newcode734 Modules/_functoolsmodule.c:734: if (PyDict_SetItem(self->cache, key, ...
5 years, 1 month ago #18
storchaka_gmail.com
4 years, 3 months ago #19
http://bugs.python.org/review/14373/diff/10053/Modules/_functoolsmodule.c
File Modules/_functoolsmodule.c (right):

http://bugs.python.org/review/14373/diff/10053/Modules/_functoolsmodule.c#new...
Modules/_functoolsmodule.c:734: if (PyDict_SetItem(self->cache, key, result) <
0) {
On 2014/04/10 07:37:29, josh.rosenberg wrote:
> Would it make sense to use PyDict_SetDefault instead of PyDict_SetItem when
> storing to the cache after calling the wrapped function?
> 
> If the return value is expensive, and multiple threads race to call the
> function, we can at least free the duplicated results and only return the
first
> value calculated. It would be really nice if we identified the case where
> multiple threads wanted the same uncached value and blocked the second and
> subsequent threads until the first thread got a result, but I imagine that
would
> add more complication and possibly decrease single threaded performance
> substantially.

Left as is, because it matches current Python implementation. We will discuss
this later for both implementations.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+