Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Cache function eval result per argument set - #988

Merged
Alexander Sher (AlexanderSher) merged 42 commits into
microsoft:masterfrom
MikhailArkhipov:argcache
Apr 19, 2019
Merged

Alexander Sher (AlexanderSher) merged 42 commits into
microsoft:masterfrom
MikhailArkhipov:argcache

Conversation

@MikhailArkhipov

Copy link
Copy Markdown

Performance

MikhailArkhipov added 30 commits March 1, 2019 09:51
This reverts commit 7ffc9db.
private readonly Dictionary<int, IMember> _argEvalCache = new Dictionary<int, IMember>();

private IMember TryEvaluateWithArguments(IPythonFunctionType fn, IArgumentSet args) {
var name = fn.DeclaringType != null ? $"{fn.DeclaringModule.Name}.{fn.Name}" : fn.Name;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be affected by #979 where the DeclaringModule of a function I had was null, so all functions of that style with the same name would get cached together.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it is a bug. I'll work on it right after this. All types should have declaring module, there are many many places where we would crash otherwise.

var key = fn.DeclaringModule.Name.GetHashCode() ^ name.GetHashCode() ^ (397 * argHash);

if (_argEvalCache.TryGetValue(key, out var result)) {
return result;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't hash collisions here make some unrelated functions have the same evaluation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions have fully qualified names like module.class.method which should have unique hashes

@AlexanderSher
Alexander Sher (AlexanderSher) merged commit cbe2a06 into microsoft:master Apr 19, 2019
Jake Bailey (jakebailey) pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
* Fix microsoft#668 (partial)

* Tests

* Revert "Tests"

This reverts commit 7ffc9db.

* Exp

* Limit concurrency

* Concurrency limit

* Drop cache after analysis

* Fix regression

* Fix test

* Cache argument eval

* Fix hash calculation

* Fix exception when there is a space before triple quote

* Fix exception
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants