Repository navigation
Conversation
|
/gemini review |
|
/gemini review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41271 +/- ##
============================================
+ Coverage 57.48% 57.64% +0.15%
Complexity 2492 2492
============================================
Files 1204 1198 -6
Lines 171265 171360 +95
Branches 2103 2103
============================================
+ Hits 98456 98784 +328
+ Misses 70157 69924 -233
Partials 2652 2652
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR optimizes MetricName by caching __hash__ and fast_name(), adding an identity fast-path in __eq__, and defining __getstate__/__setstate__ to avoid pickling cached attributes. However, _typed_metric_names is initialized on every MetricName instance without being used in this PR, and __setstate__ can silently corrupt attributes if unpickling a dict state from a prior version.
🔍 General Feedback
- Split/Unused State (
_typed_metric_names): Both the PR title andMetricName.__init__/__setstate__introduceself._typed_metric_names = {}, which is unused in this PR (its usage appears to be in #41275). This adds an unnecessary dict allocation to everyMetricNameunless paired with its consumer. - Test Coverage: Please add unit tests in
sdks/python/apache_beam/metrics/metric_test.pycoveringMetricNamepickling round-trips (pickle.loads(pickle.dumps(metric_name))) and verifying cachedhash()/fast_name()behavior (note thatmetricbase_test.pyreferenced in the PR description does not exist).
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| self.labels = labels if labels else {} | ||
| self._cached_fast_name: Optional[str] = None | ||
| self._cached_hash: Optional[int] = None | ||
| self._typed_metric_names: dict = {} |
There was a problem hiding this comment.
🟡 Medium: self._typed_metric_names is initialized here and in __setstate__ (line 129), and _TypedMetricName is mentioned in the PR title, but _typed_metric_names is never read or populated anywhere in the codebase in this PR. Unless the MetricsContainer changes from #41275 are included here, this allocates an unused empty dict on every MetricName instance and should be moved to the PR that consumes it.
| return (self.namespace, self.name, self.urn, self.labels) | ||
|
|
||
| def __setstate__(self, state): | ||
| self.namespace, self.name, self.urn, self.labels = state |
There was a problem hiding this comment.
🟡 Medium: Previously, MetricName did not define __getstate__, so its pickled representation was its 4-key instance __dict__ ({'namespace': ..., 'name': ..., 'urn': ..., 'labels': ...}). Because a 4-key dict is an iterable of length 4, unpacking state directly when unpickling a previously-serialized MetricName will silently succeed and assign the literal key strings 'namespace', 'name', 'urn', and 'labels' as the attribute values (subsequently crashing in fast_name()/__hash__() with AttributeError: 'str' object has no attribute 'items'). Handling dict states as well avoids silent data corruption across pickled states.
| self.namespace, self.name, self.urn, self.labels = state | |
| self.namespace, self.name, self.urn, self.labels = (state['namespace'], state['name'], state['urn'], state['labels']) if isinstance(state, dict) else state |
There was a problem hiding this comment.
Fixed in 5cd4e05: added isinstance(state, dict) handling in MetricName.__setstate__ and added NameTest.test_metric_name_pickle_and_legacy_dict_state.
…icName.__setstate__
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR optimizes MetricName by lazily caching the results of __hash__() and fast_name(), adding a self is other fast-path to __eq__(), and avoiding unnecessary tuple concatenation in __hash__() when labels is empty. Custom __getstate__ and __setstate__ methods are also provided (including backward-compatible handling of legacy dict pickle states) along with unit test coverage.
🔍 General Feedback
- The implementation preserves existing hash and
fast_namesemantics while significantly reducing per-lookup overhead inMetricsContainer. - Backward compatibility with previously pickled
MetricNameinstances (which serialized__dict__) is properly handled and tested inmetric_test.py. - Minor note: the PR title and summary bullet #2 still mention
_TypedMetricName/_typed_metric_names, which was removed in the second commit.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
damccorm
added
the
ready-for-human
Note: this PR was auto-generated by tooling. Before moving it out of draft, the author has done a full review of the PR, specifically evaluating it on the following criteria:
Full details of the methodology of generation and review can be found here - Apache Beam Python SDK Performance PRs Methodology & Branches Impact Ranking
If you have concerns with the PR or with the underlying methodology used to generate this, please comment below - they will be addressed by the author not the tooling :)
Summary of changes:
Optimizes
MetricNameinsdks/python/apache_beam/metrics/metricbase.py:_cached_fast_nameand_cached_hashonMetricName, and fast-pathself is otherin__eq__:hash(metric_name)built and hashed(self.namespace, self.name, self.urn) + tuple(self.labels.items())on everyMetricsContainerdictionary lookup, andfast_name()formatted strings on every call._typed_metric_namesdict onMetricName(and reset cached fields in__getstate__/__setstate__).Dataflow benchmarks showing improvement:
2026-11-12_13_27_21-138858158630926528732026-11-12_13_27_21-16378524632386198380n1-standard-8,--autoscaling_algorithm=NONE --num_workers=1 --max_num_workers=1) Cloud Dataflow pipeline running40,000,000elements (240,000,000metric ops) · 2×ParDo(Metrics.counter / MetricName.__hash__ & fast_name), built-in transform stage execution time improved from2,255,354 ms->893,556 ms(2.52xfaster,-60.4%) (total single-worker job vCPU time:3441s->1945s).Additional benchmarks
upstream/mastervsupstream/master+ this PR alone):bench_30_metric_name_cache_hash(c2feb961eef) (on branchperf/py-benchmarks-suite) — 1,000,000hash(metric_name)andfast_name()calls improved from540.16 ms->129.57 ms(4.17xfaster,-76.0%,-410.59 ms) in strict isolation.65.6%of savings,-275.43 ms): Caching_cached_fast_nameonMetricName—325.42 ms->49.99 ms(6.51xfaster,-84.6%).34.4%of savings,-144.49 ms): Caching_cached_hashonMetricName—263.74 ms->119.25 ms(2.21xfaster,-54.8%).n2-standard-4Benchmark (p28_ms:12_metric_name_hash_fastname_1m&subbench_ms:03_metrics_container_lookup_ms): On a Cloud Dataflown2-standard-4worker, 1,000,000hash(mn)+mn.fast_name()calls improved from1,056 ms->334 ms(3.16xfaster,-68.4%), and 512,000MetricsContainer.get_counter(MetricName).inc()updates improved from517 ms->180 ms(2.87xfaster).Testing
pytest sdks/python/apache_beam/metrics/metricbase_test.py sdks/python/apache_beam/metrics/execution_test.py.