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

feat(hogql): LRU cache for parsing #17438

Closed
wants to merge 2 commits into from
Closed

Conversation

mariusandra
Copy link
Collaborator

Problem

The HogQL lifecycle query takes ~3s to parse. About 2.5s of those 3s are spent waiting for antlr.
This is work that could only be done once and then cached.

image

There are other optimisations we could take in later stages, but let's tackle the biggest one first.

Changes

  • This is more of an experiment and less of a PR to merge.
  • Adds a simple LRU cache for the Antlr query parsing part.
  • Note: just the first parsing part is cached. The following parts (resolver, printer, etc) are not.
  • This will cache the query per web pod thread. With several dozen of those in operation (not sure of the exact number), we're still doing too much work. A redis-based cache might be better.
  • If we use placeholders and compose our queries in the right way, this can yield a big boost. Each query part can be cached independently.

Alternatives.

  • Find a way to make the antlr parser a lot faster (this might be a quick win, I just don't know)
  • Replace the antlr parser with a homegrown recursive descent parser (... in Rust for even more oomph)

How did you test this code?

Saw the lifecycle query load almost instantly on every second refresh.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@mariusandra mariusandra deleted the hogql-lru-experiment branch September 28, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants