You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/07/07 19:39:59 UTC
[GitHub] [beam] steveniemitz opened a new pull request, #22190: Remove locks around ExecutionStateSampler
steveniemitz opened a new pull request, #22190:
URL: https://github.com/apache/beam/pull/22190
This fixes the ExecutionStateSampler portion of #22161.
There's a couple options I've tried to remove the lock there:
- Switch activeTrackers back to a concurrent set, the problem here was that deactivating a tracker could still race with the sampler thread and double-sample a sampler. I'm not sure if this is a big enough deal to worry about?
- Rather than worry about synchronization at all, push all add/remove/sample operations into a queue and have a single thread consuming from it and maintaining activeTrackers and doing the sampling. Doing so removes the race above and simplifies the logic a lot (I think). I used the LMAX Disruptor queue to handle the multi-consumer-single-producer setup here, but you could probably use a built-in java concurrent blocking queue for this as well.
Happy to have a discussion on which to use, I figured LMAX would be fine since opencensus uses it too and will usually be included anyways, but I don't think the overhead of using a plain old java queue would be that bad either.
R: @lukecwik
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [x] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1178323588
Answers inline
> This is neat, do you have results for the various implementations you tried as to there performance impact?
The concurrent set change implementation performed about the same (anything other than the original won't even show up in profiles, it just becomes noise in the other execution). My concern was around double-counting samples at the end. If we don't care about that, it might be worth revisiting since the queue solution here is much more complicated (although conceptually cool to write :P ) There might be some concern around thread safety inside the tracker itself, I doubt in the past it was ever being use concurrently. We could also probably synchronize inside the tracker, which would almost never be contended.
>
> To answer your questions:
>
> 1. Double sampling isn't an issue even if you record the sample to the wrong bundle as long as that bundle is processing the same portion of the graph effectively. Something that we can easily do in the SDK harness implementation.
I think this is fine, at least it wouldn't have changed between different implementations.
> 2. LMAX is fine as I've had other use cases for it in mind but would rather have the benchmarks speak for themselves.
LMAX has very good latency but slightly lower throughput than a java concurrent queue. I mainly used it here to keep the queuing operation allocation free. I had found a benchmark a long time ago around a java queue vs lmax disruptor but I can't find it now.
> Other observations: A) Whatever we come up with seems valuable for the SDK harness version. Note that I created a new state sampler implementation there since the runners core one is being used by all the runners and has to have stricter synchronization while the SDK harness has a very specific thread access pattern that we can exploit for greater gains.
Agreed and I took a quick look at that already.
>
> B) One of the approaches I had considered was to have the state tracker only be added to the set when the bundle processor is created (e.g. DoFn's deserialized, graph is wired up) and removed when the entire bundle processor is destroyed. The sampling thread would then iterate over the trackers and just check to see if they are active skipping them if they aren't. This would make the bundle creation hot path faster at the cost of making the state sampling slower as the bundle processor lifetime should be pretty long as long as they are being processed regularly.
Hm this is interesting, and the number of bundle processors really should be still fairly bounded (~1000s?) so not a big deal to iterate them all. Changing it in the non-fn-execution worker might be a little more complicated than patching it up here though.
>
> C) Finally, I'm guessing that the pipeline that your running is taking too long in the synchronized portion which is causing a backlog across threads. This could be because hashset is too slow to add/remove or that the stateSampling is taking too long causing everyone else to get backed up behind that thread even though it doesn't do much. Have you tried passing in the experiment to decrease state sampling to like every 5 seconds (`--experiment state_sampling_period_millis=5000`) to see if that resolves thread contention for the most part?
I actually completely disabled the sampler and it didn't really matter (my theory was originally the same as yours, that the sampler was blocking things). The problem is just many (max 300) threads all racing (very quickly) to acquire a lock twice per bundle. The time spent actually inside the lock is very small, but according to the stats in the JFR profile I was looking at, the avg time to acquire the lock is ~80ms.
--
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
[GitHub] [beam] lukecwik commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1183624945
Run Java PreCommit
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1180873949
Run Java PreCommit
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918247075
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
Review Comment:
I think adding the threads seems worthwhile to show the cost of contention.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateTracker.java:
##########
@@ -298,7 +304,17 @@ public long getNextLullReportMs() {
return nextLullReportMs;
}
- protected void takeSample(long millisSinceLastSample) {
+ void takeSample(long millisSinceLastSample) {
+ if (SAMPLING_UPDATER.compareAndSet(this, 0, 1)) {
Review Comment:
Are you saying that there is another thread that does the sampling that isn't the ExecutionStateSampler thread?
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r916254625
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateSampler.java:
##########
@@ -149,34 +249,51 @@ public synchronized void stop() {
}
}
+ /** Wait until all published events are processed. Should only be used for testing */
+ @VisibleForTesting
+ void sync() {
+ CompletableFuture<Void> fut = new CompletableFuture<>();
+ disruptor.publishEvent(
+ (state, seq, arg0) -> {
+ state.eventType = StateEventType.SYNCHRONIZE;
+ state.syncFuture = arg0;
+ },
+ fut);
+ try {
+ fut.get();
+ } catch (Exception ex) {
+ throw new RuntimeException(ex);
+ }
+ }
+
/** Add the tracker to the sampling set. */
- synchronized void addTracker(ExecutionStateTracker tracker) {
- this.activeTrackers.add(tracker);
+ void addTracker(ExecutionStateTracker tracker) {
+ disruptor.publishEvent(
+ (state, seq, arg0) -> {
+ state.eventType = StateEventType.ADD;
+ state.tracker = arg0;
+ },
+ tracker);
}
/** Remove the tracker from the sampling set. */
void removeTracker(ExecutionStateTracker tracker) {
- synchronized (this) {
- activeTrackers.remove(tracker);
- }
-
- // Attribute any remaining time since the last sampling while removing the tracker.
- //
- // There is a race condition here; if sampling happens in the time between when we remove the
- // tracker from activeTrackers and read the lastSampleTicks value, the sampling time will
- // be lost for the tracker being removed. This is acceptable as sampling is already an
- // approximation of actual execution time.
- long millisSinceLastSample = clock.getMillis() - this.lastSampleTimeMillis;
- if (millisSinceLastSample > 0) {
- tracker.takeSample(millisSinceLastSample);
- }
+ disruptor.publishEvent(
+ (state, seq, arg0) -> {
+ state.eventType = StateEventType.REMOVE;
+ state.tracker = arg0;
+ },
+ tracker);
}
/** Attributing sampling time to trackers. */
@VisibleForTesting
- public synchronized void doSampling(long millisSinceLastSample) {
- for (ExecutionStateTracker tracker : activeTrackers) {
- tracker.takeSample(millisSinceLastSample);
- }
+ public void doSampling(long millisSinceLastSample) {
Review Comment:
I think a bunch of tests rely on the sampling happening by the time this method returns.
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r921444460
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -41,6 +41,16 @@
public class ExecutionStateSamplerBenchmark {
private static final String PTRANSFORM = "benchmarkPTransform";
+ @State(Scope.Thread)
+ public static class RunnersCoreStateTracker {
+ public ExecutionStateTracker tracker;
+
+ @Setup
+ public void setup(RunnersCoreStateSampler sharedState) {
+ tracker = new ExecutionStateTracker(sharedState.sampler);
+ }
+ }
+
@State(Scope.Benchmark)
public static class RunnersCoreStateSampler {
public final ExecutionStateSampler sampler = ExecutionStateSampler.newForTest();
Review Comment:
oh hm, yeah, this was always an issue with these tests right? I think I just accidentally fixed it for the harness one because of how the API works.
--
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
[GitHub] [beam] lukecwik commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1184890164
I restarted them.
You click `Details` on the far right and in the top left you can click on the `Rerun jobs` button and select `Rerun failed jobs`.
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1184806760
any idea how to re-run these failed actions?
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r921450365
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -41,6 +41,16 @@
public class ExecutionStateSamplerBenchmark {
private static final String PTRANSFORM = "benchmarkPTransform";
+ @State(Scope.Thread)
+ public static class RunnersCoreStateTracker {
+ public ExecutionStateTracker tracker;
+
+ @Setup
+ public void setup(RunnersCoreStateSampler sharedState) {
+ tracker = new ExecutionStateTracker(sharedState.sampler);
+ }
+ }
+
@State(Scope.Benchmark)
public static class RunnersCoreStateSampler {
public final ExecutionStateSampler sampler = ExecutionStateSampler.newForTest();
Review Comment:
these tests only ran with one thread before not many
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -41,6 +41,16 @@
public class ExecutionStateSamplerBenchmark {
private static final String PTRANSFORM = "benchmarkPTransform";
+ @State(Scope.Thread)
+ public static class RunnersCoreStateTracker {
+ public ExecutionStateTracker tracker;
+
+ @Setup
+ public void setup(RunnersCoreStateSampler sharedState) {
+ tracker = new ExecutionStateTracker(sharedState.sampler);
+ }
+ }
+
@State(Scope.Benchmark)
public static class RunnersCoreStateSampler {
public final ExecutionStateSampler sampler = ExecutionStateSampler.newForTest();
Review Comment:
these benchmarks only ran with one thread before not many
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r921455992
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -41,6 +41,16 @@
public class ExecutionStateSamplerBenchmark {
private static final String PTRANSFORM = "benchmarkPTransform";
+ @State(Scope.Thread)
+ public static class RunnersCoreStateTracker {
+ public ExecutionStateTracker tracker;
+
+ @Setup
+ public void setup(RunnersCoreStateSampler sharedState) {
+ tracker = new ExecutionStateTracker(sharedState.sampler);
+ }
+ }
+
@State(Scope.Benchmark)
public static class RunnersCoreStateSampler {
public final ExecutionStateSampler sampler = ExecutionStateSampler.newForTest();
Review Comment:
ah yeah, makes sense!
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1181711709
The precommit is failing with "cannot find symbol" warnings in unrelated code, it looks like its failed on other PRs for the same reason as well, so seems unrelated to this change.
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918239379
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateTracker.java:
##########
@@ -298,7 +304,17 @@ public long getNextLullReportMs() {
return nextLullReportMs;
}
- protected void takeSample(long millisSinceLastSample) {
+ void takeSample(long millisSinceLastSample) {
+ if (SAMPLING_UPDATER.compareAndSet(this, 0, 1)) {
Review Comment:
There's a race now between `removeTracker` removing the tracker from the concurrent set (and them sampling it) and the sampling thread enumerating it and sampling it. Previously the synchonrized block would prevent that race. This is essentially pushing the lock down to the individual tracker instances to prevent it being sampled concurrently.
--
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
[GitHub] [beam] lukecwik commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1180735040
> I updated this to use a ConcurrentHashMap-backed-set, which seems to have the best performance of the 3 options (I updated the description as well). The JMH benchmarks were super helpful here.
Finally it would be good to get some numbers for what the updated benchmark gets before and after your change.
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918286937
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
+ @Fork(1)
+ public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state, Blackhole bh)
Review Comment:
sgtm
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r921439703
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -41,6 +41,16 @@
public class ExecutionStateSamplerBenchmark {
private static final String PTRANSFORM = "benchmarkPTransform";
+ @State(Scope.Thread)
+ public static class RunnersCoreStateTracker {
+ public ExecutionStateTracker tracker;
+
+ @Setup
+ public void setup(RunnersCoreStateSampler sharedState) {
+ tracker = new ExecutionStateTracker(sharedState.sampler);
+ }
+ }
+
@State(Scope.Benchmark)
public static class RunnersCoreStateSampler {
public final ExecutionStateSampler sampler = ExecutionStateSampler.newForTest();
Review Comment:
I think these states need to be moved to RunnersCoreStateTracker like you did for HarnessStateTracker.
We would only ever expect to have one thread activating/deactivating the state. The race condition that you pointed out was between the sampling thread and one (not many) processing thread.
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918240327
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
Review Comment:
oh I didn't mean to actually commit this, I'll revert
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1180736669
> Finally it would be good to get some numbers for what the updated benchmark gets before and after your change.
It's in the description, or were you thinking of some other data?
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r920517855
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -138,33 +142,37 @@ public void testTinyBundleHarnessStateSampler(HarnessStateSampler state) throws
}
@Benchmark
- @Threads(1)
- public void testLargeBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
+ public void testLargeBundleRunnersCoreStateSampler(RunnersCoreStateSampler state, Blackhole bh)
throws Exception {
- state.tracker.activate();
+ ExecutionStateTracker tracker = new ExecutionStateTracker(state.sampler);
+ Closeable c = tracker.activate();
for (int i = 0; i < 1000; ) {
- Closeable close1 = state.tracker.enterState(state.state1);
- Closeable close2 = state.tracker.enterState(state.state2);
- Closeable close3 = state.tracker.enterState(state.state3);
+ Closeable close1 = tracker.enterState(state.state1);
+ Closeable close2 = tracker.enterState(state.state2);
+ Closeable close3 = tracker.enterState(state.state3);
// trival code that is being sampled for this state
i += 1;
+ bh.consume(i);
close3.close();
close2.close();
close1.close();
}
- state.tracker.reset();
+ c.close();
}
@Benchmark
- @Threads(1)
- public void testLargeBundleHarnessStateSampler(HarnessStateSampler state) throws Exception {
+ @Threads(10)
+ public void testLargeBundleHarnessStateSampler(HarnessStateSampler state, Blackhole bh)
+ throws Exception {
state.tracker.start("processBundleId");
Review Comment:
We don't want to have this tracker be shared across threads.
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +103,37 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
+ public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state, Blackhole bh)
throws Exception {
- state.tracker.activate();
+ ExecutionStateTracker tracker = new ExecutionStateTracker(state.sampler);
Review Comment:
We should create a JMH `@State` called `RunnersCoreStateTracker` at thread scope. Then in the setup method take in a parameter of the RunnersCoreStateSampler.
You can see an example of what I mean here: https://github.com/openjdk/jmh/blob/1f2befef92c3eb1466124f37d00f496b4105d3c5/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_29_StatesDAG.java#L134
This will allow us to move the initialization outside of the benchmark into state.
Ditto on creating `HarnessStateTracker` as well.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateTracker.java:
##########
@@ -298,7 +304,17 @@ public long getNextLullReportMs() {
return nextLullReportMs;
}
- protected void takeSample(long millisSinceLastSample) {
+ void takeSample(long millisSinceLastSample) {
+ if (SAMPLING_UPDATER.compareAndSet(this, 0, 1)) {
Review Comment:
Thanks, I missed that.
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
Review Comment:
Did we mean to have 512 threads for the tiny bundles and 16 for the large bundles or are you still meaning to revert this?
--
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
[GitHub] [beam] lukecwik commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1184892552
> > You click `Details` on the far right and in the top left you can click on the `Rerun jobs` button and select `Rerun failed jobs`.
>
> ah ok I must not have permissions to do that.
It only shows up when there are failed jobs for me. You might not see it right now because I restarted them or you could be right and its a permissions thing.
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r921442310
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
Review Comment:
that would be great as it would represent execution closer to what I see in practice
--
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
[GitHub] [beam] lukecwik merged pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik merged PR #22190:
URL: https://github.com/apache/beam/pull/22190
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1179795930
I updated this to use a ConcurrentHashMap-backed-set, which seems to have the best performance of the 3 options (I updated the description as well). The JMH benchmarks were super helpful here.
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r916254625
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateSampler.java:
##########
@@ -149,34 +249,51 @@ public synchronized void stop() {
}
}
+ /** Wait until all published events are processed. Should only be used for testing */
+ @VisibleForTesting
+ void sync() {
+ CompletableFuture<Void> fut = new CompletableFuture<>();
+ disruptor.publishEvent(
+ (state, seq, arg0) -> {
+ state.eventType = StateEventType.SYNCHRONIZE;
+ state.syncFuture = arg0;
+ },
+ fut);
+ try {
+ fut.get();
+ } catch (Exception ex) {
+ throw new RuntimeException(ex);
+ }
+ }
+
/** Add the tracker to the sampling set. */
- synchronized void addTracker(ExecutionStateTracker tracker) {
- this.activeTrackers.add(tracker);
+ void addTracker(ExecutionStateTracker tracker) {
+ disruptor.publishEvent(
+ (state, seq, arg0) -> {
+ state.eventType = StateEventType.ADD;
+ state.tracker = arg0;
+ },
+ tracker);
}
/** Remove the tracker from the sampling set. */
void removeTracker(ExecutionStateTracker tracker) {
- synchronized (this) {
- activeTrackers.remove(tracker);
- }
-
- // Attribute any remaining time since the last sampling while removing the tracker.
- //
- // There is a race condition here; if sampling happens in the time between when we remove the
- // tracker from activeTrackers and read the lastSampleTicks value, the sampling time will
- // be lost for the tracker being removed. This is acceptable as sampling is already an
- // approximation of actual execution time.
- long millisSinceLastSample = clock.getMillis() - this.lastSampleTimeMillis;
- if (millisSinceLastSample > 0) {
- tracker.takeSample(millisSinceLastSample);
- }
+ disruptor.publishEvent(
+ (state, seq, arg0) -> {
+ state.eventType = StateEventType.REMOVE;
+ state.tracker = arg0;
+ },
+ tracker);
}
/** Attributing sampling time to trackers. */
@VisibleForTesting
- public synchronized void doSampling(long millisSinceLastSample) {
- for (ExecutionStateTracker tracker : activeTrackers) {
- tracker.takeSample(millisSinceLastSample);
- }
+ public void doSampling(long millisSinceLastSample) {
Review Comment:
I think a bunch of tests rely on the sampling happening by the time this method returns and I see the sync method which is a pretty neat solution.
I wish our tests didn't invoke this stuff at all but can see why for the convenience of not having to write tests that have multiple threads.
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r921410603
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
Review Comment:
oh I was going to leave it at 10 for everything, I can switch it up though if you want.
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918231039
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
Review Comment:
I was hoping to keep all the variants the same to be able to compare directly across them so any updates like `@Threads` should be applied to them all.
It would make sense to use like 512 threads for the tiny ones and 16 for the large ones and to have a single sampler instance, one tracker per thread, and a few states per tracker since that emulates the current setup within the Dataflow worker and SDK harness.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateTracker.java:
##########
@@ -298,7 +304,17 @@ public long getNextLullReportMs() {
return nextLullReportMs;
}
- protected void takeSample(long millisSinceLastSample) {
+ void takeSample(long millisSinceLastSample) {
+ if (SAMPLING_UPDATER.compareAndSet(this, 0, 1)) {
Review Comment:
Do we need sampling/SAMPLING_UPDATER because you wanted to be able to have multiple threads activating/deactivating states within a single tracker at the same time?
Note there is only one ExecutionStateSampler thread and each tracker is associated with a single bundle at a time. To emulate this behavior we would want to have one sampler that is shared by N trackers each with their own states where N is the number of concurrent threads you want to use.
--
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
[GitHub] [beam] lukecwik commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918244026
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
+ @Fork(1)
+ public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state, Blackhole bh)
Review Comment:
Did you see that we needed the blackhole?
--
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
[GitHub] [beam] lukecwik commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1179117205
Why don't we get https://github.com/apache/beam/pull/22103 in so you can re-use the benchmarks there which would show up meaningful differences in the LMAX, concurrent set, and active tracker solutions.
If you don't got the time to review https://github.com/apache/beam/pull/22103, I can try to find another person.
--
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
[GitHub] [beam] codecov[bot] commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1178167523
# [Codecov](https://codecov.io/gh/apache/beam/pull/22190?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22190](https://codecov.io/gh/apache/beam/pull/22190?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8c4bbb7) into [master](https://codecov.io/gh/apache/beam/commit/eb5b7cc256d8d15173475cf51af758979a33bd16?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eb5b7cc) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #22190 +/- ##
========================================
Coverage 74.21% 74.22%
========================================
Files 702 702
Lines 92826 92934 +108
========================================
+ Hits 68891 68976 +85
- Misses 22667 22690 +23
Partials 1268 1268
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `83.60% <ø> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/22190?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../apache\_beam/runners/interactive/dataproc/types.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9pbnRlcmFjdGl2ZS9kYXRhcHJvYy90eXBlcy5weQ==) | `93.10% <0.00%> (-3.45%)` | :arrow_down: |
| [...n/apache\_beam/ml/gcp/recommendations\_ai\_test\_it.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvZ2NwL3JlY29tbWVuZGF0aW9uc19haV90ZXN0X2l0LnB5) | `73.46% <0.00%> (-2.05%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.01% <0.00%> (-1.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/localfilesystem.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vbG9jYWxmaWxlc3lzdGVtLnB5) | `90.97% <0.00%> (-0.76%)` | :arrow_down: |
| [...eam/runners/portability/fn\_api\_runner/execution.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL2V4ZWN1dGlvbi5weQ==) | `92.44% <0.00%> (-0.65%)` | :arrow_down: |
| [...ache\_beam/runners/dataflow/ptransform\_overrides.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9wdHJhbnNmb3JtX292ZXJyaWRlcy5weQ==) | `90.12% <0.00%> (-0.62%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/direct/executor.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kaXJlY3QvZXhlY3V0b3IucHk=) | `96.46% <0.00%> (-0.55%)` | :arrow_down: |
| [sdks/python/apache\_beam/typehints/schemas.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHlwZWhpbnRzL3NjaGVtYXMucHk=) | `94.32% <0.00%> (-0.34%)` | :arrow_down: |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.42% <0.00%> (-0.13%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/common.py](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9jb21tb24ucHk=) | `88.59% <0.00%> (-0.13%)` | :arrow_down: |
| ... and [7 more](https://codecov.io/gh/apache/beam/pull/22190/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/22190?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/22190?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eb5b7cc...8c4bbb7](https://codecov.io/gh/apache/beam/pull/22190?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918251336
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateTracker.java:
##########
@@ -298,7 +304,17 @@ public long getNextLullReportMs() {
return nextLullReportMs;
}
- protected void takeSample(long millisSinceLastSample) {
+ void takeSample(long millisSinceLastSample) {
+ if (SAMPLING_UPDATER.compareAndSet(this, 0, 1)) {
Review Comment:
Correct, it's sampled in `removeTracker` as well https://github.com/apache/beam/blob/534e99eae37e34760baa7beb432a18dc4cd148a5/runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateSampler.java#L166
--
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
[GitHub] [beam] steveniemitz commented on a diff in pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22190:
URL: https://github.com/apache/beam/pull/22190#discussion_r918250442
##########
sdks/java/harness/jmh/src/main/java/org/apache/beam/fn/harness/jmh/control/ExecutionStateSamplerBenchmark.java:
##########
@@ -103,33 +104,38 @@ public void tearDown() {
}
@Benchmark
- @Threads(1)
- public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state)
+ @Threads(10)
+ @Fork(1)
+ public void testTinyBundleRunnersCoreStateSampler(RunnersCoreStateSampler state, Blackhole bh)
Review Comment:
I figured it'd be better to be safe than sorry, it also adds a little non-trivial work to the loop which is nice for the benchmark.
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1181150673
Run Java PreCommit
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1179118167
> Why don't we get #22103 in so you can re-use the benchmarks there which would show up meaningful differences in the LMAX, concurrent set, and active tracker solutions.
>
> If you don't got the time to review #22103, I can try to find another person.
sounds good to me, I can look at it right now
--
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
[GitHub] [beam] steveniemitz commented on pull request #22190: Remove locks around ExecutionStateSampler
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on PR #22190:
URL: https://github.com/apache/beam/pull/22190#issuecomment-1184891483
> You click `Details` on the far right and in the top left you can click on the `Rerun jobs` button and select `Rerun failed jobs`.
ah ok I must not have permissions to do that.
--
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