[Perf] Optimize IsOrContainsAccessibleAttribute #19863
Conversation
Pilchie
left a comment
Pilchie
left a comment
There was a problem hiding this comment.
Thanks @Therzok!
Tagging @MattGertz for approval to help out Xamarin Studio perf.
|
Approved & thanks! |
dpoeschl
left a comment
dpoeschl
left a comment
There was a problem hiding this comment.
Code is good, just style/comments feedback.
There was a problem hiding this comment.
style: curly braces around return true;
There was a problem hiding this comment.
style: blank line between close curly brace and return false;
There was a problem hiding this comment.
If getting rid of the lambda is important, then a comment to prevent someone from putting it back might be nice.
|
Fixed comments. |
|
retest perf_correctness_prtest |
|
retest windows_release_unit64_prtest please |
|
I can't see why windows_eta_open_prtest failed, I get a 404: |
There was a problem hiding this comment.
This is reviewing an outdated commit
There was a problem hiding this comment.
There's only one commit to review :)
There was a problem hiding this comment.
this should be "var type" or "var namedType"
|
Fixed! |
There was a problem hiding this comment.
no space between method cal and open paren :)
|
retest windows_debug_unit64_prtest please |
|
Appears to have hit #19911 |
|
retest windows_release_vs-integration_prtest please |
|
Appears to have hit #19816 |
|
retest windows_debug_vs-integration_prtest please |
|
https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_vs-integration_prtest/5098/ hit a deadlock in integration tests in the project system: Main thread call-stack looks like the below. @srivatsn or @davkean have you seen this before? |
|
Hit Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpWinForms.ChangeControlProperty [FAIL]. |
|
retest windows_release_vs-integration_prtest please |
|
Fixed last styling issue. |
|
Not seen that before - I've filed dotnet/project-system#2377. @Pilchie Can you grab that dump and put it on a share? It will take me ~1 day to upload it to redmond. |
|
retest windows_release_vs-integration_prtest please |
|
Hit #15497, but for integration tests? |
|
@Therzok: Unfortunately, we've missed the deadline to get fixes like this into 15.3 without significant customer impact. Are you okay with this going into the 15.6? |
|
As long as there will be nuget roslyn releases in the meantime, we can get those into VSfM easily. |
|
@Therzok There will be on MyGet, but there won't be on NuGet.org for a while. |
|
@dotnet/roslyn-infrastructure @jimmy9988 - I just changed the target branch for this, and now it's brining 100 extra commits. Have we not been merging |
|
Tried a close/open to get the PR to drop the already merged commits, but that didn't work. @dotnet/roslyn-infrastructure Do you know what the right approach is here? |
|
@Pilchie Amend your HEAD commit and force-push. As long as you do a non-fast-forward push GitHub recomputes the effective commits. |
|
@Therzok - is that something you can do? |
|
Actually @jasonmalinowski - does it even matter? The commit has already be reviewed, and GitHub says it's a clean merge. Should we just hit merge? |
I'd personally prefer not. Seems worth it to fix the PR so that it's a human readable merge. As constructed have no idea what is being merged here. For example: PR as is has compiler changes. Pretty sure this didn't change compiler but how can I tell by looking at this? |
the AbstractRecommendationService does a filter for which symbols should be included. Given this is called from ShouldIncludeSymbol, with this change we gain: 1. Fewer iterations, as an Attribute is a type declaration, so GetMembers -> GetTypeMembers should give us fewer items to work with 2. One less lambda capture by unrolling the foreach loop.
|
Okay - I rebased and pushed (and tweaked the space between method name and open paren :) Will merge once Jenkins is done. |
|
@CyrusNajmabadi any update on #20106? |
|
retest windows_release_unit64_prtest please |
|
@Therzok Hi, sorry to bother you. It seems like this PR introduced a recent regression in completion that was reported here: #25589. Basically, as I understand this, you made 3 changes to remove the performance bottleneck in this PR:
By reading through the comments here, it seems like 3) was actually unintentional. Could you please confirm/clarify this? Thanks! As it stands, I made a PR #25622 that would put number 3) back while still keeping the first 2 optimizations (essentially calling GetMembers() on namespaces but only calling GetTypeMembers() on types to not go through all members inside each and every type), but there's a concern here. This, though likely unintentional, might have been the actual change that removed the performance problem - we don't know. But it is also possible that the performance problem would have been equally removed by the other optimizations you did and my PR would work well. Could you please weigh in on this / share some thoughts / potentially help us figure out which change might have had the crucial impact on performance and whether walking over all namespaces (until we find the first attribute) might be acceptable? Thanks a lot. |
|
Hey @Neme12. Ooops, sorry for this! I didn't intend to remove namespace recursion in the code. My test case did not have nested namespaces, but did have nested types (it's code-style enforced, so I missed this). 1 problem was that a lot of enumerator allocations were done, causing memory pressure while the code was running (we do everything in-process, so reducing allocations in hot paths is beneficial). The other problem, which increased performance, was not iterating all non-type members of a type. So, this was an unintentional change, #25622 LGTM! |
|
@Therzok Thanks, that's what I was thinking. That's good news but I think the team would still like some proof that the PR wouldn't degrade performance again. @CyrusNajmabadi said he's definitely concerned about walking all namespaces and would like to be cautious here. |
That's a failure in the original test suite that this wasn't covered. |
|
Moved discussion to the PR, I commented some other possible performance improvement that could be gained in |
|
Not all namespaces will be walked, especially when |
|
@Therzok Yes, that is correct. But walking through all namespaces would still be the worst case scenario and really depends on what sort of project/library you're working on. Some people could definitely hit this. I think we'd like to make sure that even in the worst case scenario of walking all namespaces, this is not a perf problem. |
10 participants
the AbstractRecommendationService does a filter for which symbols should be included. Given this is called from ShouldIncludeSymbol, with this change we gain:
Customer scenario
https://gist.github.com/Therzok/4a168d452c6dcd5cd9ac0ee9cf25ae0e
The stacktrace above details how these issues were found.
Risk
Risk is low. IsAttribute() can only be true on type members.
Performance impact
Improves performance
Is this a regression from a previous update?
No.
Root cause analysis:
How did we miss it? What tests are we adding to guard against it in the future?
¯_(ツ)_/¯, not a functionality change, but an optimization.
How was the bug found?
Profiling VSfM.