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

Add option to compress large ETS lookup tables #313

Closed
wants to merge 1 commit into from

Conversation

KornelH
Copy link
Contributor

@KornelH KornelH commented Oct 9, 2024

Add a configuration setting to compress large ETS lookup tables (mainly used for code navigation) to consume less memory. Please note, it will make table operations slower, therefore it might impact performance of code navigation.

Rational:
In shared environment where many instances of vscode_erlang run in the same time on large code bases, noticeable amount of memory can be consumed by this extension. In this case turning on the newly created option compressLargeEtsTables might ease the situation.

Add a configuration setting to compress large ETS lookup tables
(mainly used for code navigation) to consume less memory. Please note,
it will make table operations slower, therefore it might impact
performance of code navigation.

Rational:
In shared environment where many instances of `vscode_erlang` run in
the same time on large code bases, noticeable amount of memory can be
consumed by this extension. In this case turning on the newly created
option `compressLargeEtsTables` might ease the situation.
@KornelH
Copy link
Contributor Author

KornelH commented Oct 24, 2024

Hi @pgourlain,

What do you think about this change, do you think it worth to pull or shall it be further improved?

@pgourlain
Copy link
Owner

Hi,

Sorry for the delay.
I think that ETS compressed is only a step. And may be not enough on large projects.
I think that DETS can be also a lead, and can be activate by workspace config

your opinion ?

@KornelH
Copy link
Contributor Author

KornelH commented Oct 25, 2024

Well, I haven't thought for dets at all, but it's an interesting new idea. I will check it and see how it performs. Maybe it doesn't cause any noticeable lag (most probably) and can be used seamlessly.

In the meantime, if you are fine with the current version then I think it could be pulled. I have only one concern about it in connection to dets improvement. Is it possible the transform settings during installing of a new version of the extension if the dets version will use some more sophisticated setting instead of a simple boolean switch?

Copy link
Owner

@pgourlain pgourlain left a comment

Choose a reason for hiding this comment

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

I think usage of DETS can be add to this PR, with naming refactoring.

@@ -95,6 +95,7 @@ Support for Erlang tools, including rebar3, EUnit and Dialyzer
- `erlang.includePaths` - Include paths are read from rebar.config, and also standard set of paths is used. This setting is for special cases when the default behaviour is not enough
- `erlang.linting` - Enable/disable dynamic validation of opened Erlang source files
- `erlang.codeLensEnabled` - Enable/Disable CodeLens
- `erlang.compressLargeEtsTables` - Enable/disable compression of large ETS lookup tables (mainly used for code navigation) to consume less memory. Please note, it might impact performance.
Copy link
Owner

Choose a reason for hiding this comment

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

compressLargeEtsTables is not easy to understand for "users". and "please note it might impact performance" can affraid users

proposition : 'erlang.cacheManagement' = default, compressed, file
description : Define caching usage for code navigation, parsing result,...depending on your project size

is it make sense ?

@pgourlain
Copy link
Owner

Or if you prefer :

  1. this PR with naming and only 2 options "default, compressed"
  2. another PR with 'file' option

@KornelH
Copy link
Contributor Author

KornelH commented Oct 29, 2024

I'll update this PR with the new options ... in a few days.

@KornelH
Copy link
Contributor Author

KornelH commented Nov 21, 2024

Eventually, I created a new branch and so, a new PR #319 based on your comments. Please check it and let's continue there.

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.

2 participants