[GLUTEN-10707] Improve SparkDirectoryUtil by reduce synchronized#10708
[GLUTEN-10707] Improve SparkDirectoryUtil by reduce synchronized#10708zhztheplayer merged 2 commits intoapache:mainfrom
Conversation
|
Run Gluten Clickhouse CI on x86 |
| INSTANCE = new SparkDirectoryUtil(roots) | ||
| private def init(roots: Array[String]): Unit = { | ||
| if (INSTANCE.get() == null) { | ||
| INSTANCE.compareAndSet(null, new SparkDirectoryUtil(roots)) |
There was a problem hiding this comment.
The code is workable but the constructor of SparkDirectoryUtil may be unnecessarily called in concurrent invocations.
We can use AtomicBoolean instead if unable to avoid this.
There was a problem hiding this comment.
I'm sorry. I didn't understand what you said. Could you tell me more ?
There was a problem hiding this comment.
For example, thread 1 and thread 2 can both reach line 80 at the same time. Although only one thread will initialize the INSTANCE, another thread will still do new SparkDirectoryUtil(roots) which is not necessary.
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
beliefer
changed the title
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
zhztheplayer
left a comment
zhztheplayer
left a comment
There was a problem hiding this comment.
The get operation is still not synchronized... I am inclined to keep the current main code. Because this is not the hot code path and is not complicated. Do you agree?
| } | ||
|
|
||
| object SparkDirectoryUtil extends Logging { | ||
| private val state = new AtomicBoolean(false) |
There was a problem hiding this comment.
Let's rename it. E.g., INSTANCE_INITIALIZED
| if (state.compareAndSet(false, true)) { | ||
| INSTANCE = new SparkDirectoryUtil(roots) | ||
| } | ||
| return |
There was a problem hiding this comment.
It's not the hot code path, let's remove the if (INSTANCE == null) condition.
Just directly call compareAndSet.
There was a problem hiding this comment.
Because remove if (INSTANCE == null) will cause NPE. So I removed if (INSTANCE.roots.toSet != roots.toSet).
| def get(): SparkDirectoryUtil = { | ||
| assert(INSTANCE != null, "Default instance of SparkDirectoryUtil was not set yet") | ||
| INSTANCE | ||
| } |
There was a problem hiding this comment.
The get operation is not synchronized.
There was a problem hiding this comment.
Now, it is guarded with INSTANCE_INITIALIZED.
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
Now, the issue has been fixed. |
| def get(): SparkDirectoryUtil = { | ||
| assert(INSTANCE_INITIALIZED.get(), "Default instance of SparkDirectoryUtil was not set yet") | ||
| INSTANCE | ||
| } |
There was a problem hiding this comment.
Hi @beliefer, thank you for keeping iterating the code, but it is still problematic :(
When thread 1 reaches line 81 but hasn't yet set INSTANCE, thread 2 can pass line 88 and access INSTANCE which may give an unexpected result to caller.
I know it's a corner case, but we should make sure the new code covers what is covered by the old code completely.
There was a problem hiding this comment.
Thank you for the explanation. I will try other way.
|
Run Gluten Clickhouse CI on x86 |
beliefer
changed the title
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
@zhztheplayer Could you help me review this again ? |
| } | ||
| if (INSTANCE.roots.toSet != roots.toSet) { | ||
| if (targetRoots == null) { | ||
| targetRoots = roots |
There was a problem hiding this comment.
Perhaps just use the same name and
this.roots = roots. Just a nit. Both are fine.
|
Run Gluten Clickhouse CI on x86 |
|
@zhztheplayer Thank you! |
2 participants
What changes are proposed in this pull request?
This PR proposes to improve
SparkDirectoryUtilby reducesynchronizedFixes #10707
How was this patch tested?
GA tests.