Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote ssh performance #164

Merged
merged 20 commits into from
Oct 10, 2022
Merged

Remote ssh performance #164

merged 20 commits into from
Oct 10, 2022

Conversation

eirikpre
Copy link
Owner

@eirikpre eirikpre commented May 16, 2022

Attempt of closing down some bugs where the extension would misbehave.

  • Rewrite of DefinitionProvider, should now be properly stopped when promise resolved
  • Reference provider to use defProvider, not make a new instance.
  • No duplicate build_index
  • Fixed tests

@eirikpre eirikpre self-assigned this May 16, 2022
@eirikpre eirikpre added the bug Something isn't working label May 16, 2022
@eirikpre eirikpre requested a review from joecrop May 16, 2022 13:43
src/indexer.ts Outdated Show resolved Hide resolved
src/providers/DefinitionProvider.ts Show resolved Hide resolved
src/test/extension.test.ts Outdated Show resolved Hide resolved
@eirikpre
Copy link
Owner Author

@joecrop I tried to look into it, but could figure it out. Do you know if there is a way to avoid running Actions twice for scenarios where the branch is part of the repo and PR exists?

@eirikpre
Copy link
Owner Author

eirikpre commented May 18, 2022

@joecrop I'm seeing that the potiential_references can easily grow into millions of indexed objects.
I'm debating to set the default value of the setting to without potential_references. Or at the very least limit it if there are more than a (flat) 100 files similar to how the indexing is limited.
Ref:

if (total_files >= 1000 * this.parallelProcessing || this.forceFastIndexing) {
    return this.parser.get_all_recursive(doc, 'fast', 0);
}
if (total_files >= 100 * this.parallelProcessing) {
    return this.parser.get_all_recursive(doc, 'declaration', 0);
}
if (doc.lineCount > this.maxLineCountIndexing) {
    window.showInformationMessage(
        `The character count of ${workspace.asRelativePath(uri)} is larger than ${this.maxLineCountIndexing}. `
    ); // prettier-ignore
    return this.parser.get_all_recursive(doc, 'fast', 0);
}
// Otherwise, we parse the file with the precision requested by the user
return this.parser.get_all_recursive(doc, this.documentSymbolPrecision, 1);

EDIT* Or we can make the regular expression more restrictive

@joecrop
Copy link
Collaborator

joecrop commented May 18, 2022

@joecrop I tried to look into it, but could figure it out. Do you know if there is a way to avoid running Actions twice for scenarios where the branch is part of the repo and PR exists?

Yes, there's a way to limit this, I have done it before. Let me find out what I did in the past.

@joecrop
Copy link
Collaborator

joecrop commented May 18, 2022

EDIT* Or we can make the regular expression more restrictive

Yeah, I'm all for making the regular expressions more restrictive, but I think it might not help in performance because we would be parsing each file something like 2X more times. And if you think about it, probably 90% of the symbols in a file are indeed potential references. I think the best solution is as follows:

  1. Like you said, if the file count is small enough, leave references on unless the user explicitly disables them in settings.
    • This should be a user controlled setting, with a default of 100 like you suggested. My workspaces have 1M+ objects and 1000+ files and my computer can easily handle it, so I don't want to lose that capability.
  2. If the file count is larger than the threshold, or the setting is disabled, we ideally still want the reference provider to work. I haven't gotten around to this, but the ideal scenario is to run an indexer-like algorithm that finds all the matching symbols in all the files. This would be a relatively slow operation, but that is the tradeoff. This is how sublimeText works today.

@eirikpre
Copy link
Owner Author

EDIT* Or we can make the regular expression more restrictive

Yeah, I'm all for making the regular expressions more restrictive, but I think it might not help in performance because we would be parsing each file something like 2X more times. And if you think about it, probably 90% of the symbols in a file are indeed potential references. I think the best solution is as follows:

1. Like you said, if the file count is small enough, leave references on unless the user explicitly disables them in settings.
   
   * This should be a user controlled setting, with a default of 100 like you suggested. My workspaces have 1M+ objects and 1000+ files and my computer can easily handle it, so I don't want to lose that capability.

2. If the file count is larger than the threshold, or the setting is disabled, we ideally still want the reference provider to work. I haven't gotten around to this, but the ideal scenario is to run an indexer-like algorithm that finds all the matching symbols in all the files. This would be a relatively slow operation, but that is the tradeoff. This is how sublimeText works today.

Good points, I fully agree.

Properly fixing performance issues is to use a proper parser for all parsing that is not fast.
The proper parser can be our own ANTLR solution, or another tool that creates an AST (eg verible).
When that change happens, the parser and indexer should also be moved to the server.
Unfortunately that job is very large (and too large for me to take on at this moment 😢)

@joecrop
Copy link
Collaborator

joecrop commented May 19, 2022

Yes, there's a way to limit this, I have done it before. Let me find out what I did in the past.

@eirikpre I modified test.yml in this branch to hopefully limit this. If it doesn't do what you are hoping, you can easily tweak it now.

@joecrop
Copy link
Collaborator

joecrop commented May 19, 2022

Plugin extension for testing: systemverilog-0.13.2.vsix.zip

@joecrop
Copy link
Collaborator

joecrop commented May 31, 2022

@eirikpre I did some testing and I don't see any problems with performance or functionality with the changes in this branch.

@joecrop joecrop marked this pull request as ready for review October 9, 2022 18:13
@joecrop
Copy link
Collaborator

joecrop commented Oct 10, 2022

@eirikpre I moved the one leftover action from this PR to #186.

@joecrop joecrop merged commit 8befb9b into master Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants