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

DocumentStore refactor #702

Merged
merged 19 commits into from
Oct 18, 2022
Merged

Conversation

Techatrix
Copy link
Member

@Techatrix Techatrix commented Oct 9, 2022

Previously DocumentStore used a reference counting like approach for cleaning up unused document. This approach is unfitting because of cyclic dependencies.
As an example if A.zig imports B.zig and B.zig imports A.zig they will never be closed.
This new approach should collect all unneeded documents for cleanup.

CImports also received some care and are now managed globally instead of per document.

@leecannon
Copy link
Member

This seems to have broken completion of @import("builtin").

Completion will show public decls from build.zig and ctrl+clicking will also open build.zig.

@Techatrix
Copy link
Member Author

This seems to have broken completion of @import("builtin").

good catch! this is fixed now

leecannon
leecannon previously approved these changes Oct 10, 2022
@leecannon leecannon dismissed their stale review October 10, 2022 22:27

Possible performance impacts.

@leecannon
Copy link
Member

leecannon commented Oct 10, 2022

There is a significant performance hit associated with these changes.

TLDR: ReleaseSafe 73x longer to reach first textDocument/semanticTokens/full and 7x more memory in use

Process:

  1. Clone one of the bigger projects zigwin32
  2. Modify ZLS by changing Server.zig line 2410 (master) / line 2439 (PR) to log.info so the time to do is also displayed in release modes
  3. open vscode in the zigwin32 project with no documents open
  4. open win32.zig in the root of the project
  5. wait for the time to do textDocument/semanticTokens/full to be printed

Results:

Source Build Mode Time Memory Time change from Master Memory change from Master
Master Debug 1,885ms 178mb - -
PR Debug 49,170ms 1,139mb ~26x longer ~6x more
Master ReleaseSafe 459ms 150mb - -
PR ReleaseSafe 33,653ms 1,057mb ~73x longer ~7x more

I know this is only the initial hit when fist opening a project, but 33 seconds and a gig of ram is alot.

@Techatrix
Copy link
Member Author

I didn't expect the performance penalty to be this bad. I will just continue to work on returning to "load on demand" when i have the time and then this could be merged.

@Techatrix Techatrix marked this pull request as draft October 13, 2022 16:40
@Techatrix Techatrix marked this pull request as ready for review October 17, 2022 18:51
@Techatrix
Copy link
Member Author

The memory consumption and runtime should now be close to master.

Copy link
Member

@leecannon leecannon left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@Techatrix Techatrix merged commit 7c54ded into zigtools:master Oct 18, 2022
@Techatrix Techatrix deleted the document-store-refactor branch October 18, 2022 09:40
@Techatrix Techatrix mentioned this pull request Nov 10, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants