-
Notifications
You must be signed in to change notification settings - Fork 8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve performance of TimeUtil in different load conditions #1746
Conversation
@sczyh30 Why |
c7f8e0c
to
33fd844
Compare
private volatile long currentTimeMillis; | ||
private AtomicLong lastCheck = new AtomicLong(); | ||
private LeapArray<Statistic> statistics; | ||
private STATE state = STATE.IDLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think state should be a volatile variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, liqiangz
Here to use modifiers without a volatile
is because following considerations:
- Low frequency of switching is assumed.
- All writings to it only occurred in same thread (which maybe we should consider to remove the unnecessary CAS operation)
- Use L1/L2 cache to get better performance
So should it still be volatile? Is there any other branch I maybe lack of consideration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reply, but I still have a confusion.
When state in the daemon thread switch to IDLE, currentTimeMillis will not be updated. But when calling currentTime function in another thread, state may still be RUNNING. The getTime()
result will always be the old currentTimeMillis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, liqiangz
Both IDLE and PREPARE are treated as IDLE in getTime()
while daemon will only switch PREPARE to RUNNING when it actually started to update currentTimeMillis
.
I will remove the CAS operation which is a kind of legacy after several iterations of design. And I also will consider whether it's necessary to make it volatile in java for not strictly reordering forbidden scenario. As we know in rare condition codes like below in c++:
bool cond = true;
while (cond);
may be "optimized" to be
if (cond) {
while(true){}
}
But I am not sure it will occur in JVM's byte codes. Because it more like a bug which it's common in CPP compilers. (aha)
What do you think about the visibility latency in practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CAS operation has been removed and state is taken as volatile now.
In unit test concurrency has been raised more too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be very careful about variables shared by multiple threads. If you want to do some concurrency optimization, you must really understand what you do.
For programs that are not synchronized, if you want to judge the execution order of memory operations, it is almost impossible to get a correct conclusion . A simple way is to synchronize all variables shared by multiple threads.This is the view of the book 《java concurrency in practice》 😄
In this scenario, the value of state modified by one thread may never be seen by another thread, because we don’t know what kind of optimization the compiler and processor will perform. But because currentTimeMillis is volatile(Unlike c++, after JDK 5, JMM strictly restricts the rearrangement of volatile variables and ordinary variables by the compiler and processor), There may be no problems in this program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah totally agree with you. So CAS has been removed and volatile is marked on state while lastCheck is still a private and local variable.
- IDLE: Direct invocation state for idle conditions. - RUNNING: Legacy mode for busy conditions. - REQUEST: Temporary state from IDLE to RUNNING. Through this design TimeUtil won't cost much in idle systems any more. Signed-off-by: Jason Joo <[email protected]>
50e5417
to
7b4acda
Compare
把 |
这里的考虑是这样的,越多的配置项,看似自由,实则增加了工具的复杂性 奥卡姆剃刀原理,如无必要,勿增实体 |
我的意思是对大部分sentinel使用者来说直接内部走原生的系统调用是不会有性能问题。自适应模式对这类用户的收益不大,有时候还会造成困拢。 |
这个就看怎么评估了,是以接口方式做两种实现变成配置项(类似jvm的gc算法),还是以一个不显著(同一数量级)高于A、B两实现任一实现的方式做统一实现。 所以两个方法的确都能实现这个需求。 |
补充一点小意见:使用 PS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Nice work. This reduces the CPU footprint under low-frequent requests. Thanks for contributing! |
…ns (#1746) To achieve this by making TimeUtil have multiple running states: - IDLE: Direct invocation state for idle conditions. - RUNNING: Legacy mode for busy conditions. - REQUEST: Temporary state from IDLE to RUNNING. Through this design, TimeUtil won't cost much in idle systems anymore. Signed-off-by: Jason Joo <[email protected]>
Local test verification
Before optimization: thread CPU usage=60%-80%
Before optimization: thread CPU usage=about 8%
Before optimization: thread CPU usage=about 16% Summary: |
…ns (alibaba#1746) To achieve this by making TimeUtil have multiple running states: - IDLE: Direct invocation state for idle conditions. - RUNNING: Legacy mode for busy conditions. - REQUEST: Temporary state from IDLE to RUNNING. Through this design, TimeUtil won't cost much in idle systems anymore. Signed-off-by: Jason Joo <[email protected]>
…ns (alibaba#1746) To achieve this by making TimeUtil have multiple running states: - IDLE: Direct invocation state for idle conditions. - RUNNING: Legacy mode for busy conditions. - REQUEST: Temporary state from IDLE to RUNNING. Through this design, TimeUtil won't cost much in idle systems anymore. Signed-off-by: Jason Joo <[email protected]>
…ns (alibaba#1746) To achieve this by making TimeUtil have multiple running states: - IDLE: Direct invocation state for idle conditions. - RUNNING: Legacy mode for busy conditions. - REQUEST: Temporary state from IDLE to RUNNING. Through this design, TimeUtil won't cost much in idle systems anymore. Signed-off-by: Jason Joo <[email protected]>
Describe what this PR does / why we need it
Current design of TimeUtil may cost much more than users think in mind in idle systems.
Does this pull request fix one issue?
#1702
Describe how you did it
To achieve this by making TimeUtil have multiple running states:
By this design TimeUtil won't cost much in idle systems any more.
Describe how to verify it
Refer to the unit test
TimeUtilTest
or generate different loads to demo projects.Special notes for reviews
No