refact(core): remove lock of globalMasterInfo to optimize perf #2151
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2151 +/- ##
=============================================
+ Coverage 53.71% 68.92% +15.20%
- Complexity 652 979 +327
=============================================
Files 488 488
Lines 40078 40084 +6
Branches 5607 5608 +1
=============================================
+ Hits 21528 27628 +6100
+ Misses 16284 9799 -6485
- Partials 2266 2657 +391
... and 166 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hugegraph-core/src/main/java/org/apache/hugegraph/masterelection/GlobalMasterInfo.java
Outdated
Show resolved
Hide resolved
| public synchronized boolean isMaster() { | ||
| return this.isMaster; | ||
| public void info(boolean isMaster, String url) { | ||
| this.info = new Info(isMaster, url); |
There was a problem hiding this comment.
volatile Info info = new Info(isMaster, url);
Does volatile guarantee the atomic integrity of Info initialization? Or shall we consider to use final to guarantee to complete initialization like:
final Info info = new Info(isMaster, url);
Update：volatile could guarantee the obj init integrity, I forgot it:) However, if we don't need the obj watchable in all the time, 'final' is enough for these cases(more lightly)
There was a problem hiding this comment.
info define with volatile, so as my think, the write operator happens-before read operator, so if write operator happend first, the read operator must after the write finish
There was a problem hiding this comment.
info define with volatile, so as my think, the write operator happens-before read operator, so if write operator happend first, the read operator must after the write finish
public Info(boolean isMaster, String url) {
this.isMaster = isMaster; // ①
this.url = url; // ②
}new Info include ① & ② step, when ① is finished but ② is not, will we see an inconsistent assignments?
like Info info = new Info(true, "a"), will we get Info(true, "") or Info(false, "a")?
There was a problem hiding this comment.
the case don't happend, because method parameter be bound;
when call 1 step, the isMaster and url parameter never be modify by external logic, because the boolean is copy the origin data, and string is final;
this is a interesting question:
boolean isMaster = global.isMaster(); // 1
String url = global.url(); //2
this.info = new Info(isMaster, url); // 3
if the code run in more than one thread, the timing might look like this:
thread1: run 1 step; thread2: modify global.master; thread2: modify global.url; thread1: run 2; this case will have some concurrency problem;
but in our case, only one thread modify global, so here is no problem
There was a problem hiding this comment.
here is a “copy on write” thought;
single thread copy the data and modify it, then multi-thread read data don't consider lock
hugegraph-core/src/main/java/org/apache/hugegraph/masterelection/GlobalMasterInfo.java
Outdated
Show resolved
Hide resolved
...aph-core/src/main/java/org/apache/hugegraph/masterelection/StandardStateMachineCallback.java
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| boolean isMaster = Objects.equals(context.node(), master.node()); | ||
| String url = master.url(); | ||
| this.globalMasterInfo.set(isMaster, url); | ||
| this.globalMasterInfo.info(isMaster, url); |
There was a problem hiding this comment.
how frequently the initGlobalMasterInfo() to be called?
There was a problem hiding this comment.
frequently = 1 / heatbeat
javeme
changed the title
|
|
||
| public synchronized boolean isMaster() { | ||
| return this.isMaster; | ||
| public final void nodeInfo(boolean isMaster, String url) { |
There was a problem hiding this comment.
nice but why do we add final for the method？（for design？）
There was a problem hiding this comment.
nice but why do we add final for the method？（for design？）
final method no usefull，only express dont modify
There was a problem hiding this comment.
looks useful, the subclasses cannot override it
imbajin
changed the title
No description provided.