Cache ancestors #1120
Cache ancestors #1120
Conversation
There was a problem hiding this comment.
Thank you for that, if we believe the graph given in #1115 (comment) this is gonna be a huge performance boost. Could we check that this is really a performance boost, please ?
| @@ -1040,7 +1040,7 @@ class Past(Present): | |||
| astroid = builder.parse(data) | |||
| past = astroid["Past"] | |||
| attr = past.getattr("attr") | |||
| self.assertEqual(len(attr), 1) | |||
| self.assertEqual(len(attr), 1, attr) | |||
There was a problem hiding this comment.
| self.assertEqual(len(attr), 1, attr) | |
| assert len(attr) == 1 |
I think when using pytest this is possible (?)
There was a problem hiding this comment.
Any Ideas why this might have started failing though? I had a read through getattr but since it doesn't call ancestors directly I'm going to need to dig a little more.
There was a problem hiding this comment.
on main, attr is...
[<AssignName.attr l.3 at 0x[...]>]
with my changes this becomes...
[<AssignName.attr l.3 at 0x[...]>, <Const.int l.3 at 0x[...]>]
There was a problem hiding this comment.
Oh I was looking at the wrong getattr before... https://github.com/PyCQA/astroid/blob/f86d6e9f67e449209fe53a6e9cef4f7b55adf52a/astroid/nodes/scoped_nodes.py#L2514-L2517
There was a problem hiding this comment.
I suspect that using ClassDefs and InferenceContexts as keys was a bit naive: they aren't Singletons and don't implement __hash__ and __eq__ so I am only caching specific instances by object id instead of detecting when the ancestors or contexts are equivalent.
| # FIXME: should be possible to choose the resolution order | ||
| # FIXME: inference make infinite loops possible here | ||
| yielded = {self} | ||
| yielded = OrderedDict() |
There was a problem hiding this comment.
I think this is necessary for Python 3.6 to preserve the order of yielded ancestors: a plain dict achieves the same thing from Python 3.7+
Once I get the tests to pass I want to try this against the example given in pylint-dev/pylint#4079 and then perhaps retry #1115 too. Are there some codebases to run I also wondered if I should try and measure what the extra memory cost will be: I know there's prior art for this from https://rtpg.co/2020/10/12/pylint-usage.html |
|
@dwo Are you still interested in picking this PR back up or should we close it? |
|
Damn, I forgot about this and would have liked to spend more time on it... will close for now. |
|
I still think this would indeed be very valuable. |
Steps
Description
Type of Changes
Related Issue