Use Atomic vars in multithreaded env. ++num and num++ operations aren… #16356

dk2k · 2024-10-16T19:33:04Z

Brief description of changes.
For volatile num:
++num transformed to num.incrementAndGet()
num++ transformed to num.getAndIncrement()

…'t atomic, can't use them with volatile vars Signed-off-by: Dmitry Kryukov <[email protected]>

github-actions · 2024-10-16T20:37:49Z

❌ Gradle check result for 20ea733: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dblock

I think this is a good change, @andrross double check with me?

github-actions · 2024-10-16T23:24:24Z

❕ Gradle check result for 20ea733: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2024-10-16T23:24:54Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.10%. Comparing base (ec7b652) to head (20ea733).

Files with missing lines	Patch %	Lines
...eldcaps/TransportFieldCapabilitiesIndexAction.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #16356      +/-   ##
============================================
+ Coverage     72.00%   72.10%   +0.10%     
- Complexity    64817    64880      +63     
============================================
  Files          5307     5307              
  Lines        302660   302663       +3     
  Branches      43724    43724              
============================================
+ Hits         217931   218238     +307     
+ Misses        66906    66578     -328     
- Partials      17823    17847      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrross · 2024-10-17T16:59:22Z

server/src/main/java/org/opensearch/common/util/PlainIterator.java

-    // keyword in order to ensure visibility. Note that it is fine to use `volatile` for a counter in that case given
-    // that although nextOrNull might be called from different threads, it can never happen concurrently.
-    private volatile int index;
+    private AtomicInteger index = new AtomicInteger();


The comment here explains why the original author thought a volatile primitive is acceptable. Why do you think that is incorrect?

Also, the primitive int will use 4 bytes, while an AtomicInteger object will use at least 16 bytes (if my understanding of how Java allocates Objects is correct...), so this change will result in at least a small increase in memory footprint. The difference is may be completely negligible but I don't know that for sure.

Also, the primitive int will use 4 bytes, while an AtomicInteger object will use at least 16 bytes (if my understanding of how Java allocates Objects is correct...)

It's worse than that. The 16 bytes is just for the object, but it needs to maintain the internal 4-byte value, plus objects are 8-byte aligned so it'll be 24 bytes plus an 8-byte memory address for a total of 32 bytes. Some guy spent way too long investigating this a decade ago.

@andrross
"volatile" keyword means you want to see the actual value in multithreaded env - i.e. no caching of the value.
But operations like num++ and ++num aren't atomic. And the other thread can see the outdated value.
I think I need to pass 0 to the constructor of Atomic integer in line 48. How do you think?

@dk2k As you say, atomicity is required for incrementing, but that never happens in a multithreaded context (per the comment).

All that is required for reading is getting the latest value. If you catch it in the middle of an update you may get +1 or -1 but that's really no different than grabbing it right before or right after an (atomic) update as far as the other thread is concerned.

If you think the original author's claim that nextOrNull won't ever be called concurrently is wrong, please explain why.

dbwiddis · 2024-10-17T16:47:59Z

...hmarks/src/main/java/org/opensearch/benchmark/time/NanoTimeVsCurrentTimeMillisBenchmark.java

@@ -28,7 +29,7 @@
 @State(Scope.Benchmark)
 @SuppressWarnings("unused") // invoked by benchmarking framework
 public class NanoTimeVsCurrentTimeMillisBenchmark {
-    private volatile long var = 0;
+    private final AtomicLong var = new AtomicLong(0);


I'm not sure this particular change is an improvement. JMH runs a single thread per fork, so the volatile long is sufficient, and the extra overhead might impact the benchmark and might skew the results.

Since the accessing of a long variable is intended as an upper bound for the other calls, I'm somewhat skeptical it would retain that "upper bound" meaning with additional overhead vs. the other two method calls.

Looking at this again, I think the intent of this benchmark is to calculate the difference between accessing Millis vs. Nanos, and remove the overhead of accessing a long (as an upper bound) for a fair comparison of the remainder of the actual code.

So this change needs to be removed.

dbwiddis · 2024-10-17T16:52:54Z

server/src/main/java/org/opensearch/common/util/PlainIterator.java

-    // keyword in order to ensure visibility. Note that it is fine to use `volatile` for a counter in that case given
-    // that although nextOrNull might be called from different threads, it can never happen concurrently.
-    private volatile int index;
+    private AtomicInteger index = new AtomicInteger();


The comment here seems to indicate that volatile is for visibility but the incrementing won't happen concurrently. This adds unneeded overhead.

server/src/main/java/org/opensearch/action/search/QueryPhaseResultConsumer.java

dbwiddis · 2024-10-17T17:08:55Z

server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java

@@ -277,7 +278,7 @@ private ShardRouting nextRoutingOrNull(Exception failure) {
        }

        private void moveToNextShard() {
-            ++shardIndex;
+            shardIndex.incrementAndGet();


I don't think the change is needed here, as this won't ever be accessed concurrently. It's only ever accessed on initialization and then after receiving a response to a transport message, so there's a forced network/transport delay between successive calls to this method.

…sultConsumer.java Co-authored-by: Daniel Widdis <[email protected]> Signed-off-by: Dmitry Kryukov <[email protected]>

github-actions · 2024-10-18T08:11:25Z

❌ Gradle check result for 8b62369: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dbwiddis

Re-reviewing after the last commit.

The change to QueryPhaseResultsComsumer is good and the latest commit improves it.
I think the changes to TransportFieldCapabilitiesIndexAction and PlainIterator are not needed, but would not veto an approval by other maintainers.
I think the change to the NanoTimeVsCurrentTimeMillisBenchmark invalidates the benchmark and I will not approve without this change being removed from the PR.

Use Atomic vars in multithreaded env. ++num and num++ operations aren…

20ea733

…'t atomic, can't use them with volatile vars Signed-off-by: Dmitry Kryukov <[email protected]>

dblock approved these changes Oct 16, 2024

View reviewed changes

opensearch-ci-bot mentioned this pull request Oct 16, 2024

[AUTOCUT] Gradle Check Flaky Test Report for SharedClusterSnapshotRestoreIT #15845

Open

andrross reviewed Oct 17, 2024

View reviewed changes

dbwiddis requested changes Oct 17, 2024

View reviewed changes

Update server/src/main/java/org/opensearch/action/search/QueryPhaseRe…

8b62369

…sultConsumer.java Co-authored-by: Daniel Widdis <[email protected]> Signed-off-by: Dmitry Kryukov <[email protected]>

dbwiddis requested changes Oct 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Atomic vars in multithreaded env. ++num and num++ operations aren… #16356

Use Atomic vars in multithreaded env. ++num and num++ operations aren… #16356

dk2k commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

dblock left a comment

github-actions bot commented Oct 16, 2024

codecov bot commented Oct 16, 2024

andrross Oct 17, 2024

dbwiddis Oct 17, 2024 •

edited

Loading

dk2k Oct 18, 2024 •

edited

Loading

dbwiddis Oct 18, 2024

dbwiddis Oct 17, 2024

dbwiddis Oct 17, 2024

dbwiddis Oct 17, 2024

dbwiddis Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024

dbwiddis left a comment

Use Atomic vars in multithreaded env. ++num and num++ operations aren… #16356

Are you sure you want to change the base?

Use Atomic vars in multithreaded env. ++num and num++ operations aren… #16356

Conversation

dk2k commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

dblock left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024

codecov bot commented Oct 16, 2024

Codecov Report

andrross Oct 17, 2024

Choose a reason for hiding this comment

dbwiddis Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

dk2k Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

dbwiddis Oct 18, 2024

Choose a reason for hiding this comment

dbwiddis Oct 17, 2024

Choose a reason for hiding this comment

dbwiddis Oct 17, 2024

Choose a reason for hiding this comment

dbwiddis Oct 17, 2024

Choose a reason for hiding this comment

dbwiddis Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2024

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis Oct 17, 2024 •

edited

Loading

dk2k Oct 18, 2024 •

edited

Loading

dbwiddis Oct 17, 2024 •

edited

Loading