You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "arunpandianp (via GitHub)" <gi...@apache.org> on 2025/01/23 05:45:48 UTC
[PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
arunpandianp opened a new pull request, #33737:
URL: https://github.com/apache/beam/pull/33737
Example profile showing the relevant stacks <img width="1688" alt="Screenshot 2025-01-22 at 9 44 53 PM" src="https://github.com/user-attachments/assets/1a2e7d69-e429-4956-8cc0-0eec1629f14c" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #33737:
URL: https://github.com/apache/beam/pull/33737#discussion_r1926622299
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/OutputObjectAndByteCounter.java:
##########
@@ -18,27 +18,28 @@
package org.apache.beam.runners.dataflow.worker.util.common.worker;
import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
import org.apache.beam.runners.core.ElementByteSizeObservable;
import org.apache.beam.runners.dataflow.worker.counters.Counter;
import org.apache.beam.runners.dataflow.worker.counters.CounterBackedElementByteSizeObserver;
import org.apache.beam.runners.dataflow.worker.counters.CounterFactory;
import org.apache.beam.runners.dataflow.worker.counters.CounterFactory.CounterMean;
import org.apache.beam.runners.dataflow.worker.counters.CounterName;
import org.apache.beam.runners.dataflow.worker.counters.NameContext;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
import org.checkerframework.checker.nullness.qual.Nullable;
/** An {@link ElementCounter} that counts output objects, bytes, and mean bytes. */
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
public class OutputObjectAndByteCounter implements ElementCounter {
+
// Might be null, e.g., undeclared outputs will not have an
// elementByteSizeObservable.
private final ElementByteSizeObservable<Object> elementByteSizeObservable;
private final CounterFactory counterFactory;
- private Random randomGenerator = new Random();
Review Comment:
what about just leaving existing code for testing and just using ThreadLocalRandom here (it extends Random)?
or changing this to final and having a constructor that allows injecting it instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
Posted by "arunpandianp (via GitHub)" <gi...@apache.org>.
arunpandianp commented on code in PR #33737:
URL: https://github.com/apache/beam/pull/33737#discussion_r1926639508
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/OutputObjectAndByteCounter.java:
##########
@@ -18,27 +18,28 @@
package org.apache.beam.runners.dataflow.worker.util.common.worker;
import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
import org.apache.beam.runners.core.ElementByteSizeObservable;
import org.apache.beam.runners.dataflow.worker.counters.Counter;
import org.apache.beam.runners.dataflow.worker.counters.CounterBackedElementByteSizeObserver;
import org.apache.beam.runners.dataflow.worker.counters.CounterFactory;
import org.apache.beam.runners.dataflow.worker.counters.CounterFactory.CounterMean;
import org.apache.beam.runners.dataflow.worker.counters.CounterName;
import org.apache.beam.runners.dataflow.worker.counters.NameContext;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
import org.checkerframework.checker.nullness.qual.Nullable;
/** An {@link ElementCounter} that counts output objects, bytes, and mean bytes. */
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
public class OutputObjectAndByteCounter implements ElementCounter {
+
// Might be null, e.g., undeclared outputs will not have an
// elementByteSizeObservable.
private final ElementByteSizeObservable<Object> elementByteSizeObservable;
private final CounterFactory counterFactory;
- private Random randomGenerator = new Random();
Review Comment:
I think if we assign ThreadLocalRandom.current() to the variable, the variable will point to ThreadLocalRandom of the thread that did the assignment and all threads will use the same object.
We could change it to a Suplier<Random> and pass it via constructor, I think it is not that different from the instance method returning Random. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #33737:
URL: https://github.com/apache/beam/pull/33737#discussion_r1928368937
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/OutputObjectAndByteCounter.java:
##########
@@ -18,27 +18,28 @@
package org.apache.beam.runners.dataflow.worker.util.common.worker;
import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
import org.apache.beam.runners.core.ElementByteSizeObservable;
import org.apache.beam.runners.dataflow.worker.counters.Counter;
import org.apache.beam.runners.dataflow.worker.counters.CounterBackedElementByteSizeObserver;
import org.apache.beam.runners.dataflow.worker.counters.CounterFactory;
import org.apache.beam.runners.dataflow.worker.counters.CounterFactory.CounterMean;
import org.apache.beam.runners.dataflow.worker.counters.CounterName;
import org.apache.beam.runners.dataflow.worker.counters.NameContext;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
import org.checkerframework.checker.nullness.qual.Nullable;
/** An {@link ElementCounter} that counts output objects, bytes, and mean bytes. */
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
public class OutputObjectAndByteCounter implements ElementCounter {
+
// Might be null, e.g., undeclared outputs will not have an
// elementByteSizeObservable.
private final ElementByteSizeObservable<Object> elementByteSizeObservable;
private final CounterFactory counterFactory;
- private Random randomGenerator = new Random();
Review Comment:
makes sense, I thought ThreadLocalRandom internally resolved each time by thread. I missed the private constructor and current method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle merged PR #33737:
URL: https://github.com/apache/beam/pull/33737
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33737:
URL: https://github.com/apache/beam/pull/33737#issuecomment-2608914462
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow] Use ThreadLocalRandom instead of Random in OutputObjectAndByteCountert o reduce contention [beam]
Posted by "arunpandianp (via GitHub)" <gi...@apache.org>.
arunpandianp commented on PR #33737:
URL: https://github.com/apache/beam/pull/33737#issuecomment-2608912774
R: @scwhittle
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org