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

perf(query) Memoize the part of the logical plan tree traversal for reduced memory allocation and faster planning #1874

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

amolnayak311
Copy link
Contributor

@amolnayak311 amolnayak311 commented Oct 29, 2024

Pull Request checklist

  • The commit(s) message(s) follows the contribution guidelines ?
  • Tests for the changes have been added (for bug fixes / features) ?
  • Docs have been added / updated (for bug fixes / features) ?

Current behavior :

Though there is no expected change in the behavior for the end users, the memory allocation for methods getPushdownShards, isTargetSchemaChanging and QueryUtils.makeAllKeyValueCombos is high. This high memory allocation can be reduced by memoizing the responses which are repeatedly called by recursing the logical plan tree

With no caching enabled the JMH tests for materialize give following results

[info] Do not assume the numbers tell you what you want them to tell.
[info] Benchmark                                        Mode  Cnt   Score    Error  Units
[info] PlannerBenchmark.benchmarkMaterializePlan        avgt    5  20.819 ± 13.145  ms/op
Screenshot 2024-10-29 at 3 03 08 PM Screenshot 2024-10-29 at 3 02 34 PM

As seen above, getPushdownShards, isTargetSchemaChanging show up as two hotspots for memory allocation profiling.

New behavior :

After memoizing the responses, the hotspots are no longer visible. The planner's materialize is 2-3X improvement in materialize times and then two calls don't show up in allocation hotspots anymore.

[info] Do not assume the numbers tell you what you want them to tell.
[info] Benchmark                                        Mode  Cnt  Score   Error  Units
[info] PlannerBenchmark.benchmarkMaterializePlan        avgt    5  7.642 ± 3.868  ms/op
Screenshot 2024-10-29 at 3 18 34 PM

@amolnayak311 amolnayak311 merged commit 7f008e7 into filodb:develop Nov 1, 2024
1 check passed
amolnayak311 added a commit to amolnayak311/FiloDB that referenced this pull request Nov 11, 2024
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