Introduce CachedToStringMetricKey and CachedToStringMetricName for Improved Performance #127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FuRyanf
force-pushed
the
FuRyanf/optimize-metric-key-name-cache
branch
from
d6088f0 to
334ce0e
Compare
FuRyanf
force-pushed
the
FuRyanf/optimize-metric-key-name-cache
branch
4 times, most recently
from
4ed7daa to
429075a
Compare
FuRyanf
force-pushed
the
FuRyanf/optimize-metric-key-name-cache
branch
from
429075a to
a64ed43
Compare
coderplay
reviewed
Sep 13, 2024
| public String toString() { | ||
| return stepName() + ":" + metricName(); | ||
| } | ||
|
|
||
| public static MetricKey create(@Nullable String stepName, MetricName metricName) { | ||
| return new AutoValue_MetricKey(stepName, metricName); | ||
| return new CachedToStringMetricKey(new AutoValue_MetricKey(stepName, metricName)); |
There was a problem hiding this comment.
ha, I thought that we will create a CachedToStringMetricKey , that replaces the AutoValue_MetricKey instead of wrapping the AutoValue_MetricKey. But either way should be fine.
There was a problem hiding this comment.
Not a fan of autovalue. :)
There was a problem hiding this comment.
Haha, I see what you mean! Initially, I also thought about directly creating a CachedToStringMetricKey to replace AutoValue_MetricKey, but I realized that AutoValue provides its own implementations of .equals and .hashcode, which I wanted to preserve. That’s why it evolved into the wrapper approach—my goal was to minimize changes while keeping the core behavior intact.
coderplay
reviewed
Sep 13, 2024
| public String toString() { | ||
| return getNamespace() + ":" + getName(); | ||
| } | ||
|
|
||
| public static MetricName named(String namespace, String name) { | ||
| checkArgument(!Strings.isNullOrEmpty(namespace), "Metric namespace must be non-empty"); | ||
| checkArgument(!Strings.isNullOrEmpty(name), "Metric name must be non-empty"); | ||
| return new AutoValue_MetricName(namespace, name); | ||
| return new CachedToStringMetricName(new AutoValue_MetricName(namespace, name)); |
coderplay
reviewed
Sep 13, 2024
| } | ||
|
|
||
| public static MetricName named(Class<?> namespace, String name) { | ||
| checkArgument(namespace != null, "Metric namespace must be non-null"); | ||
| checkArgument(!Strings.isNullOrEmpty(name), "Metric name must be non-empty"); | ||
| return new AutoValue_MetricName(namespace.getName(), name); | ||
| return new CachedToStringMetricName(new AutoValue_MetricName(namespace.getName(), name)); |
coderplay
approved these changes
Sep 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description:
Introduce
CachedToStringMetricKeyandCachedToStringMetricNamefor Improved PerformanceSummary:
This PR introduces two new classes,
CachedToStringMetricKeyandCachedToStringMetricName, which cache the result of thetoString()method to improve performance in scenarios wheretoString()is frequently called.Key Changes:
MetricKeythat caches thetoString()result upon construction, reducing overhead from repeatedtoString()calls in performance-sensitive areas.CachedToStringMetricKey, this class wraps aroundMetricNameand caches itstoString()value for efficiency, while still delegating all other methods to the originalMetricName.equals()andhashCode()Overrides: To ensure consistency in collections,equals()andhashCode()were overridden to delegate these comparisons to the underlyingMetricNameandMetricKeyinstances. This ensures correct behavior when these objects are used as keys in maps.@NullableParameters: Updated theequals()method to handle@Nullableparameters correctly, following Java’s contract forequals()and ensuring compatibility with null comparisons.@MemoizedinMetricKeyandMetricNamewith the new cached classes, removing the overhead caused by the@Memoizedpattern.Motivation:
The use of
@Memoizedfor thetoString()method in the auto-generatedMetricKeyandMetricNameclasses introduces unnecessary overhead due to its thread-safe memoization pattern, which includes additional synchronization logic. This pattern creates atransient volatile String toStringfield and performs a synchronized check with everytoString()call. This can degrade performance, especially whentoString()is frequently invoked in performance-sensitive parts of the metrics system.By caching the
toString()value at construction, we eliminate this overhead, leading to significant performance improvements. Benchmarks have shown that even directly computingtoString()is faster than the memoized implementation. The new approach optimizes for frequenttoString()invocations without the cost of synchronization.Changes:
@Memoizedannotations fromMetricKeyandMetricName.MetricKey.create()to useCachedToStringMetricKeyandMetricNameconstruction to useCachedToStringMetricName.toString()and ensured consistent usage across the codebase to avoid potential issues with mixed types in collections.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.