[STORM-1038] Upgraded netty to 4.x

[ooasis]

Upgraded the netty transportation layer to 4.x to take advantage of its memory management efficiency.

[HeartSaVioR]

@hsun-cnnxty
Great work. Since it can affect performance I recommend you to include benchmark test result.
You can refer #521 (comment) to how to do it.

[HeartSaVioR]

@hsun-cnnxty
Btw, Netty 4.0.31 was released 8 days ago, so we may also want to check this out to test it works.
And we'd like to see memory usages with Netty 3.x / 4.x to confirm 4.x is superior to 3.x.

[ooasis]

Good suggestion. I will get to work on them as soon as I get some time. I am also curious to verify the memory efficiency claimed by 4.x

[ooasis]

Upgraded to latest 4.0.31.Final and changed the buffer allocation to let Netty choose the best default implementations based on the platform.

The implementations are determined by following properties:
io.netty.noPreferDirect: false to prefer off-heap and true for heap allocation. Default to false
io.netty.allocator.type: default to pooled or unpooled on Android platform

[revans2]

@hsun-cnnxty, The changes look good to me. Please upmerge to the latest code. Then I would like to run some performance tests on it to see how it compares to the current code. Please also look at shading. In storm-core we are shading netty now, and I would like to be sure that it is still shaded correctly.

If you have any questions on how to do this please let me know and I will be happy to help out.

[ooasis]

@revans2, just merged with the latest master. I don't have a decent storm cluster for performance test. With a small local cluster on single machine. I had tried https://github.com/yahoo/storm-perf-test and did not see any difference in memory consumption after the upgrade (It was configured in a way to make sure there is inter-worker communication using Netty). Hope your test can reveal more information.

The shading config is updated in pom.xml with

 <relocation>
<pattern>io.netty</pattern>
<shadedPattern>org.apache.storm.netty</shadedPattern>
</relocation>

Hope that's all I need to do.

-thanks

[rfarivar]

Could you please expand the imports?

Also below.

[revans2]

If this is not needed we should remove it, not comment it out.

[revans2]

Everything looks really good now. just a few minor nits. I still have not found time to run some performance tests, but I will try to do that today.

[revans2]

I just ran some performance tests using https://github.com/apache/storm/blob/master/examples/storm-starter/src/jvm/storm/starter/ThroughputVsLatency.java

I ran with 4 workers on a MacBookPro and the numbers don't look good for netty4.

With the older code I was able to do 20,000 sentences/second at
99%-ile: 43 ms
99.9%-ile: 99 ms
min: 8.8 ms
max: 177 ms
mean: 16 ms

CPU Utilization over 30 second interval:
user: 159,718 ms
sys: 49,670 ms

But with the netty4 patch it could only handle 17,000/sec and the latency was much worse
99%-lie: 148 ms
99.9%-lie: 230 ms
min: 9.2 ms
max: 332 ms
mean: 32 ms

CPU:
user: 162,984 ms
sys: 55,867 ms

To have the netty4 patch have similar latency we needed to only run at 14,000 sentences per second.

99%-lie: 38 ms
99.9%-lie: 55 ms
min: 8.8 ms
max: 76 ms
mean: 16 ms

CPU:
user: 152,865 ms
sys: 52,535 ms

Unless we can get the numbers to be close to or better than the netty3 implementation I cannot let this in.

[ooasis]

Cool, at least we get some numbers to compare. I will see if there is default setting need to be changed for netty 4.

[revans2]

@hsun-cnnxty I hope it is something like that. You should be able to run the tests yourself too. They are not that complex. I build

mvn clean install -DskipTests
cd storm-dist/binary
mvn clean package
tar -xzvf ./target/apache-storm-0.11.0-SNAPSHOT.tar.gz
cd apache-storm-0.11.0-SNAPSHOT

I then run a small single node cluster

./bin/storm dev-zookeeper &
./bin/storm nimbus &
./bin/storm supervisor &
./bin/storm ui &
./bin/storm logviewer &

Once it is all up and ready you can run the test.

./bin/storm jar ./examples/storm-starter/storm-starter-topologies-0.11.0-SNAPSHOT.jar storm.starter.ThroughputVsLatency $THROUGHPUT 4 5 wc-test | tee results.txt

It will output metrics about the running test every 30 seconds for 5 mins. Some of the numbers are in nanoseconds and others are in milliseconds so pay attention to them. I like to vary the throughput and look to see when it cannot keep up any more to get an idea of the maximum throughput the setup can handle, and what the latency is for a given throughput so we can see how they compare to each other.

[ooasis]

With some refactoring, now it can sustain throughput of 20,000 /sec which it was not able to before. But latency at 20,000 /sec is still much higher than 3.x (5+ times). I will continue to investigate.

[ooasis]

@revans2

Just merged code from master and seems there is performance degradation with recent changes. I noticed that it not only affects this branch, but also the master branch. Here are the comparison on my laptop for running the perf test with only 5,000/sec throughput with code from master.

Before (git hash: 3db9680)
netty3-r5000.txt

Now (git hash: bd396b3)
netty3-new-r5000.txt

Note: when running the latest code, my CPU is almost 100% busy which may explain why it was so much worse. When running the "old" code, my CPU still had 10% idle time.

-thanks

[revans2]

Yes I am aware of it. We are in the process of merging with the JStorm project, and part of this merger involves moving most of the clojure code to java. In all likelihood the degradation is due to reflection happening on the critical path because many new places in the code are calling into java that were not doing so before. I plan on doing some profiling soon and submit some fixes, but it is very much a moving target so I have not been very motivated to do so. The code for the netty messaging layer should stay mostly the same between 1.0 and 2.0 for this, so if you want to do your work based off of the 1.0 branch and show performance comparisons there being the same I would totally accept that as proof that the code is good.

[harshach]

@hsun-cnnxty we would like to get this into 1.x-branch as well as master. Did you get a chance to look at @revans2 comment above . It will be great if you can address the comment and up merge your patch.

[ooasis]

I am currently on vacation and will be back in two weeks. Will work on it as soon as I am back home.

-thanks

[HeartSaVioR]

After rebasing, could you do the performance test against 1.x branch? The status of master branch is a WIP so we would be more convenient with 1.x branch.

[ooasis]

Sure.

[ooasis]

As this PR is for master, new PR #1591 is created for 1.x-branch. Performance tests to be done soon.

[ooasis]

@HeartSaVioR@harshach

I posted performance test results on #1591.

[kishorvpatil]

@hsun-cnnxty, I think this is great change to have. Any way we can implement this as a plugin for us to switch between current implementation? It took substantial amount to get the current version stabilized. So making it plug and play would help us switch between implementations in case of issues.

[ooasis]

@kishorvpatil that's an interesting idea. You mean a feature flag to toggle between 3.x and 4.x? I will investigate the possibility.

Btw, I have moved the work to #1591.

-thanks