Uh oh!
There was an error while loading. Please reload this page.
[SPARK-15246][Core] Fix code style and improve volatile for SPARK-4452 - #13020
[SPARK-15246][Core] Fix code style and improve volatile for SPARK-4452#13020lianhuiwang wants to merge 1 commit into
Conversation
| // Number of elements read from input since last spill | ||
| @volatile protected def elementsRead: Long = _elementsRead | ||
| protected def elementsRead: Long = _elementsRead |
There was a problem hiding this comment.
can you comment on why the changes are safe/correct?
There was a problem hiding this comment.
It has only one thread to use this method. So we can remove volatile.
lianhuiwang
commented
May 10, 2016
cc @davies |
| if (!isSpilled) { | ||
| 0L | ||
| } else { | ||
| _elementsRead = 0 |
There was a problem hiding this comment.
I delete this code in order to _elementsRead is called by one thread. So we do not need to make _elementsRead volatile.
davies
commented
May 10, 2016
Is this really an hot fix (serious bug or breaking tests)? |
lianhuiwang
commented
May 10, 2016
@davies I think it is not an hot fix. So i have updated PR. thanks. |
SparkQA
commented
May 10, 2016
Test build #58203 has finished for PR 13020 at commit
|
davies
commented
May 10, 2016
LGTM |
| protected def forceSpill(): Boolean | ||
| // Number of elements read from input since last spill | ||
| @volatile protected def elementsRead: Long = _elementsRead |
There was a problem hiding this comment.
It's not meaningful to make a method volatile anyway right? the fact that it's not an error makes me think I could be missing something. No objection
There was a problem hiding this comment.
Yes, I think you understand correctly. we can remove volatile of elementsRead method because there is only one thread to use it.
rxin
commented
May 11, 2016
Merging in master/2.0. Thanks. |
## What changes were proposed in this pull request? 1. Fix code style 2. remove volatile of elementsRead method because there is only one thread to use it. 3. avoid volatile of _elementsRead because Collection increases number of _elementsRead when it insert a element. It is very expensive. So we can avoid it. After this PR, I will push another PR for branch 1.6. ## How was this patch tested? unit tests Author: Lianhui Wang <lianhuiwang09@gmail.com> Closes#13020 from lianhuiwang/SPARK-4452-hotfix. (cherry picked from commit 9f0a642) Signed-off-by: Reynold Xin <rxin@databricks.com>
What changes were proposed in this pull request?
After this PR, I will push another PR for branch 1.6.
How was this patch tested?
unit tests