[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-23 Thread revans2
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2261 I spent the last few hours running more tests and I get the same results. I am not too concerned about it. The overhead appears to be rather low if any. --- If your project is set up for it, you

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-23 Thread revans2
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2261 From a code perspective the changes look fine to me. It would be nice to possibly clean up the load aware grouping Interface some. From a performance perspective it is a bit of a wash from

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-22 Thread roshannaik
Github user roshannaik commented on the issue: https://github.com/apache/storm/pull/2261 The impact of loadAware that you show here seems inline with what I have seen. Encouraging to see these improvements. Reviewed only the core chooseTasks() implementation and left one comment

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-22 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 @revans2 @roshannaik I know you're busy, but could you have time to take a look at the change? I guess it is clear improvement and I provide raw numbers to see the difference. --- If your

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-22 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 Raw numbers are here: https://gist.github.com/HeartSaVioR/5e80ab3a58b3e8cf40bab9c6da482639 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-22 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 I got back to 8f63d5a which doesn't touch any interfaces and do same tests: Grouping | transferred (messages) | transfer rate (message/s) | spout_transferred | spout_acks |

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-22 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 Now I have another numbers to persuade this patch. I just take same approach to what @Ethanlm is done with his patch #2270 Performance testing on ConstSpoutNullBoltTopo with ACKing

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-08 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 I explored and applied more changes: * change ArrayList to pre-allocated array (they're only allocated from `prepare()`) * fixed length of `chooses` length: 1000 * at first I set

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-07 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 I just take opposite approach, pushing updated load mapping when load updater timer is activated. We no longer need any tricks or optimizations to reduce checking, and even no need to check

[GitHub] storm issue #2261: STORM-2678 Improve performance of LoadAwareShuffleGroupin...

2017-08-04 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2261 Now I introduce 'skip checking update count' to avoid calling System.currentTimeMillis() every time, but it has clear trade-off, we should call AtomicInteger.incrementAndGet() every time. I