fix: Fix data race in memory profiling #1727
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1727 +/- ##
============================================
+ Coverage 56.12% 58.65% +2.53%
- Complexity 976 1138 +162
============================================
Files 119 129 +10
Lines 11743 12692 +949
Branches 2251 2375 +124
============================================
+ Hits 6591 7445 +854
- Misses 4012 4058 +46
- Partials 1140 1189 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| used -= size; | ||
| long newUsed = used.addAndGet(-size); | ||
| if (newUsed < 0) { | ||
| logger.warn("Used memory is negative: " + newUsed); |
There was a problem hiding this comment.
Should probably make this an error message (or maybe even throw an exception since this is a pretty fatal condition?)
There was a problem hiding this comment.
I guess this situation is likely to happen, the memeory acquired by MemoryPool.grow may be less than the actual request. Filed #1733
There was a problem hiding this comment.
I changed this to use error logging rather than a warning. I'd prefer not to throw an exception because many jobs will run to completion without issue even if this variable does go negative.
| /** The id uniquely identifies the native plan this memory manager is associated to */ | ||
| private final long id; | ||
|
|
||
| private final TaskMemoryManager internal; | ||
| private final NativeMemoryConsumer nativeMemoryConsumer; | ||
| private long used; | ||
| private final AtomicLong used = new AtomicLong(); |
spark/src/main/java/org/apache/spark/CometTaskMemoryManager.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Oleks V <[email protected]>
Which issue does this PR close?
Closes #1726
Rationale for this change
Fix a data race that caused negative memory use to be reported
What changes are included in this PR?
Use AtomicLong instead of long to track used memory.
How are these changes tested?
Manually.