This repository was archived by the owner on Apr 14, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 133
Mitigate some of #495 via MRO member selection and analysis set optimization #517
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optimization may not work. Code below gets hash codes and collapses them into hash sets, which means that two buckets arrays with different amount of items will be considered equal. I can't say if that is correct or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the comparers are the same, should there ever be a situation where two sets have different contents that are the same? To me that seems related to the issue of keys sometimes changing after being added (that
Debug.Fail
), except that this specific check is costly to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It maybe possible as a result of some
AnalysisSet
merges or if there is more than one defaultBucket
instance with0
hash set.Btw, have you tested if there is any significant benefit of this optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm not sure how merging matters for this. Wouldn't
Buckets.Count
be updated when that occurs? Or does a union not actually do any unioning and instead concatenates?I don't have any numbers, but it seemed like it shifted some CPU time out to other places when I was working on fastai. Namely, I started with the count checks, then saw a lot of CPU time used on accessing
Count
(since it's put behind a lock), then moved the access above.Most of the CPU time in my testing comes from set comparison and tuples getting too large, which I wasn't able to cleanly limit. The MRO lookup change curbs things in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen sets with empty buckets under debugger, but I have no idea how to reproduce it and I don't know if that is a buggy result or an expected one. This code is quite muddy. @MikhailArkhipov , what is your opinion?
Just for the reference. Thin locks are really fast, but I don't know if we have thin or fat lock here.
GetHashCode
is overridden, but it isn't the only thing that can promote thin lock to fat lock.Yes, MRO fix is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The locking I'm referring to is here: https://github.com/Microsoft/python-language-server/blob/cb1c3dfbad1c7a521857a5dae8eea7c94225a096/src/Analysis/Engine/Impl/AnalysisHashSet.cs#L95-L106
Now that I look more closely, most other functions grab
Bucket
at the start. The original function here did (by grabbinglBuckets
and maybe grabbingrBuckets
). Perhaps I should have moved those upward and operated on them instead of introducing two extra possible locks.I'm happy to remove some of the more suspect length checks if you think this may be an issue (but some of them should still be valid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, the thin vs fat locking is past
lock
and more to do with object layout, so me showing where the locking is doesn't matter. Oops.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different numbers of items (types) in buckets and yet equality seem odd to me. I am not overly concerned with keeping correct number of items in buckets since collecting types from various type of calls is generally not what we want to be doing. Ex,
def f(a): return a
called withint
now hasint
return. Called then withstr
it is suddenly show as returningint, str
. But that's not true, it is not returning combo or a union. And then user adds code calling with bool and it now it returns triplet... Which has nothing to do with the nature of the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New analysis has special types for call arguments and generics so the said function is considered returning type of its argument so no need to collect possible outcomes.
f(1).
will show completion forint
members andf('a').
will yieldstr
members without need to hold unions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM. I'd build insiders and let people try it out.