use an lru_cache to speed up recursive ast parsing #60
Conversation
|
For the |
|
Does it fix the performance regression completely, or if not what's the gain you are seeing? |
|
I just tried comparing with pytest-run-parallel 0.3.1 on the scipy example and it looks like this PR completely fixes the regression, within measurement noise. |
|
I can confirm that Full traceback: |
|
I'm not able to reproduce that. That said, I think this is coming from the change to use a I wanted to add a test but since I can't reproduce the issue Ralf was seeing it's hard to do that confidently. |
|
This approach is a bit slower than the initial approach, collection takes 2s instead of 0.55s on scipy's |
|
Are you doing anything special to trigger that |
Not really. It's reproducible on macOS as well, with It shouldn't matter that I use @@ -232,7 +233,12 @@ pytest = "*"
hypothesis = "*"
threadpoolctl = "*"
pooch = "*"
-pytest-run-parallel = ">=0.3.0"
+
+[feature.free-threading.pypi-dependencies]
+pytest-run-parallel = { path = "../../tmp/pytest-run-parallel", editable = true }
[feature.free-threading.tasks]
build-nogil = { cmd = "spin build --build-dir=build-nogil -C-Dblas=blas -C-Dlapack=lapack -C-Duse-g77-abi=true", cwd = "scipy", env = { CC = "ccache $CC", CXX = "ccache $CXX", FC = "ccache $FC" } }And then: |
|
The last commit seems to work, while still helping performance (just less so). On macOS arm64 I'm seeing collection times for the whole SciPy test suite of:
|
|
Thanks for your patience - it turns out I was testing an old scipy commit. I can reproduce after a |
|
I spent a little time this morning trying to make a test case that triggers this so I can add a regression test but couldn't manage to trigger it. The |
|
I checked out this branch and tried with scikit-learn by running: It runs in 19 to 21s both for 214f68f of this branch and 3d65bf7 of the current For reference, not using the plugin: It runs in 4 to 5s. So I don't see any significant effect of this PR w.r.t. the use case described in #56. Did I miss anything? |
|
@ogrisel thank you so much for testing. This is why it's a bad idea to push code close to mignight! The try/except obscured that I was causing a cache miss on every call - moving to a frozenset fixes that. |
|
I see 12 seconds on |
|
@ngoldbaum I confirm it works for me as well with the latest commit of this branch: 4s which is very close to the collection duration with the plugin disabled. Good job! |
rgommers
left a comment
rgommers
left a comment
There was a problem hiding this comment.
LGTM now - works on the SciPy test suite, fully fixes the performance issue, and looks pretty clean.
Thanks @ngoldbaum and @ogrisel!
|
I think we should release this as 0.4.3 - let's discuss later today @ngoldbaum |
|
I confirm that this much nicer to use |
|
Thanks again for the report - I'll try not to implement any accidentally O(N!) (I think?) algorithms anytime soon... We'll try to get a bugfix release out this week. |
3 participants
Fixes #56
I'm not sure whether it makes more sense to use
functools.cacheand just not worry about memory overhead.lru_cacheseemed to be sufficient to speed up the example from sciki-learn in #56.