You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by arunmahadevan <gi...@git.apache.org> on 2016/03/29 07:04:07 UTC
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
GitHub user arunmahadevan opened a pull request:
https://github.com/apache/storm/pull/1272
[STORM-1662] Reduce map lookups in send_to_eventlogger
Reducing map lookup in send_to_eventlogger can improve performance when when a spout emits in a tight loop.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arunmahadevan/storm STORM-1662
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1272.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1272
----
commit 3844395d83cc17a7b1eba80e6db9174a52ad4423
Author: Arun Mahadevan <ai...@hortonworks.com>
Date: 2016-03-29T04:43:57Z
[STORM-1662] Reduce map lookups in send_to_eventlogger
Reducing map lookup in send_to_eventlogger can improve performance when when a spout emits in a tight loop.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1272: [STORM-1662] Reduce map lookups in send_to_eventlo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1272
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-203841803
In result, sorry I changed my vote to +0 for now.
I think we should make the test result stable before taking any actions.
- test several times and take average or mean
- test longer
- test with dedicated (idle, not dev., no GUI) machine
If we can get the stable result before releasing 1.0.0 is in progress, we can include this as 1.0.0. But it shouldn't block releasing 1.0.0.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-202726953
@arunmahadevan
No effect with eventlogger=0 is what I assume, so I'm saying that's weird...
I tested on roshan's topology with no modifying.
Maybe I need to clean up something with each test, or test with your topology.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-202725876
@HeartSaVioR actually this patch has no effect with eventlogger=0. I assume you modified BasicTopology to emit a constant long. I also restart all the storm processes before each run and let it run for 12 mins and compare the last 10 min window stats.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-203836195
@arunmahadevan
I've just done with performance test, and the result is a bit different what I expected.
This patch affects the performance even though event logger is set to 0 thought I can't understand the result.
This performance test is done with your topology + modification for logging performance.
(https://gist.github.com/HeartSaVioR/0e2555633a5f7d12cb68)
I restarted all Storm processes for every test.
[eventlogger-performance-test-STORM-1662-excel.xlsx](https://github.com/apache/storm/files/197260/eventlogger-performance-test-STORM-1662-excel.xlsx)
Since test is done with my dev. machine (MBP), I really would like to borrow a dedicated machine and try this performance test again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-202728331
>I tested on roshan's topology with no modifying.
I also tried that and it was producing weird results, so I changed it to emit a constant long and restarted storm processes after each run.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-202723884
+1 if it's same to https://github.com/arunmahadevan/storm/commit/7eae5ec9f63cee82c49980a3bedf5f0dfe4e3a8d.
I already applied this patch and test, and it looks good.
Btw, I may need to have dedicated server during test, cause when last performance test for me this patch doesn't increase throughput when eventlogger=1 but this patch **increase throughput when eventlogger=0**.
(Tested with roshan's topology, I didn't test with your topology. I can't even work while benchmarking since I should run that for my work machine... ;( )
> event logger executors=1, origin
```
uptime: 540 duration: 30 secs
spout emitted: 24016500 (total: 420641440) emitted/sec (in duration): 800550.0
bolt emitted: 24016620 (total: 420639920) emitted/sec (in duration): 800554.0
uptime: 570 duration: 30 secs
spout emitted: 24392600 (total: 445034040) emitted/sec (in duration): 813086.6666666666
bolt emitted: 24392480 (total: 445032400) emitted/sec (in duration): 813082.6666666666
uptime: 600 duration: 30 secs
spout emitted: 24524700 (total: 469558740) emitted/sec (in duration): 817490.0
bolt emitted: 24524620 (total: 469557020) emitted/sec (in duration): 817487.3333333334
```
> event logger executors=0, origin
```
uptime: 541 duration: 30 secs
spout emitted: 27652560 (total: 441035220) emitted/sec (in duration): 921752.0
bolt emitted: 27653040 (total: 441034260) emitted/sec (in duration): 921768.0
uptime: 571 duration: 30 secs
spout emitted: 25150820 (total: 466186040) emitted/sec (in duration): 838360.6666666666
bolt emitted: 25150280 (total: 466184540) emitted/sec (in duration): 838342.6666666666
uptime: 601 duration: 30 secs
spout emitted: 25021760 (total: 491207800) emitted/sec (in duration): 834058.6666666666
bolt emitted: 25021860 (total: 491206400) emitted/sec (in duration): 834062.0
```
> event logger executors=1, applied arun's patch
```
uptime: 541 duration: 30 secs
spout emitted: 24319300 (total: 426107880) emitted/sec (in duration): 810643.3333333334
bolt emitted: 24319140 (total: 426106420) emitted/sec (in duration): 810638.0
uptime: 571 duration: 30 secs
spout emitted: 24394800 (total: 450502680) emitted/sec (in duration): 813160.0
bolt emitted: 24395420 (total: 450501840) emitted/sec (in duration): 813180.6666666666
uptime: 601 duration: 30 secs
spout emitted: 24425140 (total: 474927820) emitted/sec (in duration): 814171.3333333334
bolt emitted: 24424720 (total: 474926560) emitted/sec (in duration): 814157.3333333334
```
> event logger executors=0, applied arun's patch
```
uptime: 541 duration: 30 secs
spout emitted: 26572320 (total: 464465820) emitted/sec (in duration): 885744.0
bolt emitted: 26572680 (total: 464464560) emitted/sec (in duration): 885756.0
uptime: 571 duration: 30 secs
spout emitted: 26573580 (total: 491039400) emitted/sec (in duration): 885786.0
bolt emitted: 26573140 (total: 491037700) emitted/sec (in duration): 885771.3333333334
uptime: 601 duration: 30 secs
spout emitted: 26586520 (total: 517625920) emitted/sec (in duration): 886217.3333333334
bolt emitted: 26587000 (total: 517624700) emitted/sec (in duration): 886233.3333333334
```
Super odd result anyway, so I'd really like to see stable numbers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1662] Reduce map lookups in send_to_eve...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1272#issuecomment-202711879
@HeartSaVioR could you take a look ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---