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

Cache additional methods in MetricLookup / SemanticModelLookup #1438

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Oct 2, 2024

Similar to the previous PRs, this PR caches a few additional methods. I did a little bit of memory profiling using a large manifest, and total memory allocation did not change significantly. I believe this is because these additional caches reference objects that are already stored in ValidLinkableSpecResolver or the cached objects have low overhead.

This is also a common method that is called during query resolution. The memory
used by the cache is small since the number of measures is on the order of ~100
in large manifests.
@cla-bot cla-bot bot added the cla:yes label Oct 2, 2024
@plypaul plypaul marked this pull request as ready for review October 2, 2024 16:22
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -67,6 +67,9 @@ def __init__(self, model: SemanticManifest, custom_granularities: Dict[str, Expa
# Cache for defined time granularity.
self._time_dimension_to_defined_time_granularity: Dict[TimeDimensionReference, TimeGranularity] = {}

# Cache for agg. time dimension for measure.
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to include a period after agg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since it is an abbreviation.

@plypaul plypaul merged commit 2ee4e79 into main Oct 2, 2024
40 checks passed
@plypaul plypaul deleted the p--short-term-perf--13 branch October 2, 2024 19:44
courtneyholcomb pushed a commit that referenced this pull request Oct 9, 2024
…1438)

Similar to the previous PRs, this PR caches a few additional methods. I
did a little bit of memory profiling using a large manifest, and total
memory allocation did not change significantly. I believe this is
because these additional caches reference objects that are already
stored in `ValidLinkableSpecResolver` or the cached objects have low
overhead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants