Making Pylint faster 2 #519
Conversation
Assign statements can only appear in multiline bodies (like function definitions) and in the value side of assign statements (e.g. `b = 4` is an assign node contained in `a = b = 4`), so there is no need to check other nodes for them.
Return statements can only appear in multiline bodies (like function definitions), so there is no need to check other nodes for them.
Yield statements can only appear in multiline bodies (like function definitions) and in lambdas, so there is no need to check other nodes for them.
astroid/scoped_nodes.py
Outdated
| for child_node in self.body: | ||
| yield from child_node._get_assign_nodes() | ||
|
|
||
| def _get_yield_nodes_skip_lambdas(self): |
There was a problem hiding this comment.
Do you think it would make sense to make some of these methods into Mixin classes to reduce code duplication? I know this won't work for functions which use node specific attributes, but I see _get_yield_nodes_skip_lambdas is exactly the same in multiple locations.
|
Thanks for tackling this @nickdrozd ! My main concern is, as you said, that these changes starts to sacrifice the readability of the code for the performance, with the code becoming extremely specialized and too clever for its sake. We should try to strike a balance between these two, so some degree of code generalisation should be kept around. I have two points for discussion for this PR. As Bryce mentioned this patch introduces some duplicate methods which are also a bit hard to understand without having the context of why they were implemented like this (for new contributors for instance). I suggest we should also keep them separated from the node implementations themselves, since right now performance tricks mingle with the rest of the code. I don't have something in particular in mind, maybe we can implement these in, say, something like The other point is that even with these specialisations, as mentioned in your other PR, I think that us not having a way to verify in an automated way that the performance is not hurt, through some new feature or change we're adding, will definitely be a disadvantage. We should invest some effort into this before coming up with new specialisations, since we're not going to run yappi ourselves periodically. A third point that came in my mind during writing this comment is more of a question really. I wonder if it's worth looking into rewriting some of the astroid's stack in a more efficient language rather than doing it all in Python. After a point, there won't be a lot of specialized changes that could be applied to get a significant boost in performance. I know @ceridwen also thought about that at some point. If we could rewrite the AST in C (maybe a CPython extension or Cython, something along these lines), we could introduce run these tree functions from C rather than from pure Python. This could lead to the biggest performance gain with the effect that the code could be kept super simple and not overly specialized. Unfortunately I definitely don't have time for a big project like this one. |
|
Something that I would like to do (but I also don't have time for big projects at this point) is to create a corpus of Python code for testing astroid against, or least run pytest on all the standard library modules to make sure it doesn't crash. The same corpus could be used for profiling/performance testing. As it is, we have two problems. First, all the benchmarks that anyone has ever run astroid/pylint against are awfully specific, which is fine for users who are really only concerned how fast astroid is on their particular projects but not good for astroid development because we don't know if optimizing for one project might hurt performance in others. Second, we don't have any continuous integration set up for performance regressions so any future changes in the code base could easily slow down astroid without their authors realizing it. As for this patch in particular, have we dropped support for Python 2.7? If so, someone should really update the readme and PyPi tags because they still claim support for 2.7. (Dropping support for 2.7 before it officially EOLs in 2020 seems like a bad idea to me, but I won't reopen that if it's decided.). Beyond that, like @brycepg says, there's a lot of code duplication that could probably be reduced, at least, with mixins. |
|
I agree with @ceridwen on the fact that we need performance regressions. @ceridwen regarding Python 2 support, we decided to drop it in pylint-dev/pylint#1763. This is restricted to the future major releases, pylint 2.0 and astroid 2.0 We'll still release bug fixes for the current astroid 1.6 and pylint 1.8 until Python 2 is EOL, so probably until January 2020. |
|
One problem I've found with CI performance regression tests is that Appveyor/Travis CI are not made to provide performance consistency. (Benchmarks, travis CI issue). Plus they would be too slow, the CI is already nearly unbearably slow I think It wouldn't be too difficult for me to set up a folder in pylint for integration tests and curate some number of projects to run pylint against using I'll open a new issue for performance integration tests, but I don't think they should block this issue; It's pretty obvious that astroid shouldn't search assignment statements for return nodes. |
|
I'd say this change generally makes code faster (definitely not slower): The corpus is here: |
|
Thanks for tackling the performance benchmarks Bryce! Regarding the output, what are the values from before and after this patch? Would be great if you'd offer a small breakdown on those values. Regarding the PR itself, this is not blocked by the performance benchmarks, but by my other point regarding the specialization, separation & code duplication. I'm waiting on @nickdrozd to follow up. Overall I think the patch and its improvements are great to have, just needs some polishing before getting it in. |
|
It appears I unfortunately do not have access to a dedicated server for proper performance testing and these measurements were taken in a Paravirtual machine. |
|
Okay, I figured out what I think is a reasonably elegant way to consolidate all that repeated code, the @PCManticore It would certainly be nice if a bird flew by and dropped Pylint-rewritten-in-C into our hands, but short of that I doubt it will ever happen. And is it really needed anyway? I think there are still plenty of performance gains to be had just from changes to the existing Python code. |
|
@nickdrozd Definitely, this would be a huge project for which none of us has time nor the incentive to do it. Although it would be interesting to think about it if we're aiming for pylint to become as fast as pyflakes & co with regard to linting. That is, even if we optimize pylint as much as we can, it will never be as fast as the likes of pyflakes & co. |
There was a problem hiding this comment.
Looks great now! Thanks for the efforts @nickdrozd ! Left two small comments, let me know when you're ready with the merge.
astroid/mixins.py
Outdated
|
|
||
|
|
||
| class MultiLineBlockMixin: | ||
| def _get_multi_line_blocks(self): |
There was a problem hiding this comment.
We could use a decorators.cachedproperty here.
| @@ -131,3 +131,41 @@ def real_name(self, asname): | |||
| raise exceptions.AttributeInferenceError( | |||
| 'Could not find original name for {attribute} in {target!r}', | |||
| target=self, attribute=asname) | |||
|
|
|||
|
|
|||
| class MultiLineBlockMixin: | |||
There was a problem hiding this comment.
Can you add a docstring on why we have this mixin?
`_multi_line_block_fields` is a list of strings indicating which fields contain multi-line blocks. `_get_multi_line_blocks` dynamically accesses these attributes the first time it is called and then caches the resulting references so as to avoid repeated expensive attribute lookups.
|
@PCManticore done |
|
Thanks @nickdrozd ! Looking forward to the next improvements! :) |
My last PR contained what I would describe as compiler-style
optimizations, mainly loop unrolling and getting rid of runtime type
checking and dynamic attribute handling. I applied those changes more
or less mechanically, without really understanding the purpose of all
the functions involved.
In spite of those improvements, the last yappi graph still showed
major hotspots in
_get_return_nodes_skip_functionsand_get_yield_nodes_skip_lambdas(and also, to a lesser extent, in_get_assign_nodes). Thinking about how those functions were beingused, I realized that a lot of unnecessary work was still being done.
Consider
returnstatements, for instance. Most node types (e.g.function calls, assign statements, comprehensions) cannot legally
contain them, so if we're searching for
returnstatements, we canpass those nodes by immediately. The only nodes that can contain
returnstatements are nodes with multi-line bodies, likeIfandFunctionDef, and thus they are the only ones that need to implement_get_return_nodes_skip_functions. Similar reasoning holds for theother changes here. (I'm sure there is plenty of cleverness to be applied
further.)
To test these changes, I ran
pylint(withpylint'spylintrcfile) against
pycodestyle.py, as before, as well as againstpylintitself. These cases differ in two import ways: 1)
pylintis amulti-file project with nested directories, while
pycodestyle.pyisjust a single file, and 2)
pylintis written to passpylintwithout any errors, while
pycodestyle.pyraises lots ofpylinterrors.
Despite their differences,
pylintran about twenty seconds faster inboth cases:
pycodestyle.py
Before
After
pylint
Before
After
Now, as these optimizations become increasingly specialized, there
comes a risk that the code will become increasingly inelegant.
Certainly some elegance has already been sacrificed by replacing
nodes_of_classwith type-specific variations. But I think thatpylint's slowness is a major usability issue, especially for largeprojects, and therefore the tradeoff is definitely worth it.