Improve visibility for DataCarrier. #3726
Conversation
wu-sheng
added
core feature
wu-sheng
requested review from
hanahmily,
zhaoyuguang,
ascrutae,
JaredTan95,
kezhenxu94 and
peng-yongsheng
|
The old thread graph is like this.
As we have improved many other points for the performance, this seems a missing one. As this was reported, and no one submitted PR, I am trying to do this. |
|
@yantaowu I think you mentioned this before, could you check about this too? |
|
@lkxiaolou As you was working on optimizing DataCarrier multiple times, could you help on rechecking this too? |
| } | ||
|
|
||
| public void setItem(Object item) { | ||
| this.item = item; |
There was a problem hiding this comment.
Item set volatile may reduce the producer's performance
There was a problem hiding this comment.
Yes. The key is I want to reduce the visibility to consumer thread. I want to check whether it is real having that issue.
There was a problem hiding this comment.
The point is, if the visibility issue happens, it affect the performance more, the queue will be blocked longer than required.
|
Is it a real different in our case? I used to check on that too |
|
I'm missing the context of PR a little. Do you intend to make consumer threads to observe the change of "queue" with the help of |
|
@hanahmily I was being reported by community, the consumers are not fully working. That is the context |
What's the state of these consumers? I think that's a clue for us to find out why. |
We just know the threads are not busy from the graph. Don't know why. As we have reduced the number of threads, should be a problem. |
I had tested this PR. The issue mentioned before has resolved, but the CPU usage rate is more than our own implementation. |
Hi @yantaowu |
|
@dmsolr He is running cherry pick test on old release, 6GA. The CPU usage is only a reference. I will try to do a comparison test between this and his, which is using blocking queue. |
|
@hanahmily @kezhenxu94 @dmsolr I run a local benchmark, and YES. In blocking mode(the OAP is using this mode now), the ArrayBlockingQueue has ~30% performance enhancement. I write a fake Buffer class to run this test And this is my benchmark public class ChannelWritePerformanceTest {
private static Buffer buffer = new Buffer(2000, BufferStrategy.BLOCKING);
private static Buffer2 buffer2 = new Buffer2(2000, BufferStrategy.BLOCKING);
@Benchmark
public static void testBuffer1() {
for (int i = 0; i < 200; i++) {
buffer.save(new Object());
}
buffer.obtain(new ArrayList(2000));
}
@Benchmark
public static void testBuffer2() {
for (int i = 0; i < 200; i++) {
buffer2.save(new Object());
}
buffer2.obtain(new ArrayList(2000));
}
public static void main(String[] args) throws RunnerException {
Options opt = new OptionsBuilder()
.include(ChannelWritePerformanceTest.class.getSimpleName())
.forks(1)
.warmupIterations(1)
.measurementIterations(5)
.build();
new Runner(opt).run();
}
}The following is test report Do you have any suggestions? Do we need to update this? Such as using BlockingQueue in blocking mode? |
|
When I changed the codes to insert more data, interesting things happened. |
|
As @yantaowu told me offline, they are using |
|
I dig more into the ArrayBlockingQueue source code, I noticed there is a misguide from the user feedback. So after I remove the |
|
Furthermore, by using the master codes of DataCarrier, we have much higher throughput. The following result is using the master code as |
|
For following all these tests, unless we have very specific evidence showing there is across threads visibility issue, I think we should close this PR for now.
|
wu-sheng
added
TBD
|
OK. An update, the ArrayBlockingQueue processing the across threads visibility by using the lock. |
|
I will submit another PR to provide a new buffer implementation for oap server side first. The client side needs more research. |
As we received several reports about the usage of DataCarrier queue consume thread is not very high, which could cause CPUs are not fully used. I add a more layer to the DataCarrier queue to avoid the array element change is invisible across threads.
This is a compatible change. I hope someone could verify this in test or product envs. This should make performance of OAP better with less latency in process data.