Cache building operations in DataflowPlanBuilder
#1448
Conversation
plypaul
force-pushed
the
p--short-term-perf--22
branch
from
bf1803c
to
450e45e
Compare
plypaul
force-pushed
the
p--short-term-perf--23
branch
from
5a2200e
to
0c254f2
Compare
plypaul
force-pushed
the
p--short-term-perf--22
branch
from
450e45e
to
d44fd4a
Compare
plypaul
force-pushed
the
p--short-term-perf--23
branch
from
0c254f2
to
33b1a10
Compare
| @@ -100,6 +106,10 @@ | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
|
|
|||
| _MEASURE_RECIPE_CACHE_SIZE = 1000 | |||
There was a problem hiding this comment.
Curious if we've put much thought into what the size should be? I guess we'll just experiment and tune it accordingly?
There was a problem hiding this comment.
These are based on rough sizes of semantic manifests and performance tests on the larger ones we've seen. We do need to encapsulate all these caches into a single object and add instrumentation.
| def __init__( # noqa: D107 | ||
| self, find_source_node_recipe_cache_size: int = 1000, build_any_metric_output_node_cache_size: int = 1000 | ||
| ) -> None: | ||
| self._find_source_node_recipe_cache = LruCache[FindSourceNodeRecipeParameterSet, FindSourceNodeRecipeResult]( |
There was a problem hiding this comment.
It would be pretty cool if we could use the new LruCache while still using decorator syntax to maintain readability of the methods that are cached. But I guess it's also debatable if that's more readable (e.g., it obscures the cache code if you end up needing to debug something there). And probably not worth the effort it would take to make it work anyway...
There was a problem hiding this comment.
Yeah, the obscuring of the dependencies with a decorator would make it harder to follow.
plypaul
force-pushed
the
p--short-term-perf--22
branch
from
d44fd4a
to
23754cb
Compare
plypaul
force-pushed
the
p--short-term-perf--23
branch
from
33b1a10
to
8d704e1
Compare
plypaul
force-pushed
the
p--short-term-perf--22
branch
from
23754cb
to
ef0eddc
Compare
plypaul
force-pushed
the
p--short-term-perf--23
branch
from
8d704e1
to
8829989
Compare
plypaul
force-pushed
the
p--short-term-perf--23
branch
from
8829989
to
a3d5d63
Compare
This PR adds a few LRU caches to handle building operations within the
DataflowPlanBuilder. Since the same metric may be used multiple times in a derived metric (or between queries), there can be significant performance improvements. Please view by commit as there were signature / type changes to making the changes cleaner.