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

refactor: diagnostic cache to an ets table #348

Closed

Conversation

NJichev
Copy link
Collaborator

@NJichev NJichev commented Dec 20, 2023

Refactors the Diagnostic Cache from using an Agent to a ETS table.

Agent.get(cache, & &1)
cache
|> GenServer.call(:all)
|> to_map()
Copy link
Collaborator Author

@NJichev NJichev Dec 20, 2023

Choose a reason for hiding this comment

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

This is purely done to preserve the old interface of the cache, I originally refactored all the places that use it but decided to preserve it as is. Let me know what you think would be better, I think using the new list structure directly at some places might be a performance benefit other than that the old nested map structure is more convenient for use.

There was also the approach of storing it in ets as maps but it turned out to be a hassle since there's no update like interface like Map.update Edit: There is actually :ets.select_replace/2 which could do the job.

@mhanberg
Copy link
Collaborator

Thanks for taking a crack at this! I assume this is because of my TODO comment in the file.

Couple of things:

  • Unless you read from the ets table directly, you aren't really going to see any read concurrency benefits, as its still bottle necked through the gen server.

  • I added that comment originally naively, but i'm not sure if if this really is any kind of bottle neck.

There is now open telemetry in the project, complete with a docker-compose to spin up an otel collector and grafana tempo to view traces. It might be benficial to wrap the diagnostics cache in some spans and use it for a bit to see i the latency, then with an ets implementation and compare.

let me know if you want to try the tracing and i can help you through it.

@NJichev
Copy link
Collaborator Author

NJichev commented Dec 20, 2023

I agree let's do it like this, I think I'll close this one for now then, since there are a couple of more improvements to be made, for example moving the reads to the table to the caller process, the problem I had was that tests spin up their own cache to keep the tests async so having them be separate processes made this harder.

Maybe I could register the ets table in the lsp assigns as well to improve this so that I don't need to lookup the table ref every time.

@NJichev NJichev closed this Dec 20, 2023
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