STORM-3100 : Introducing CustomIndexArray to speedup HashMap lookups #2711

roshannaik · 2018-06-10T12:38:54Z

Added Unit Tests
Ran below perf numbers on my mid 2015 macbook pro for single worker.

Ran these perf topos to check perf impact:

1) ConstSpoutIdBoltNullBoltTopo With 1 Acker

Before: Throughput= 1,067,128 /sec . Latency = 1.038 ms
After: Throughput= 1,294,511 /sec . Latency = 1.037 ms

Throughput improved ~21%. No signifiant change in latency.

2) ConstSpoutIdBoltNullBoltTopo With 0 Acker

Before: Throughput= 4,389,133
After: Throughput= 4,748,744 /sec

Throughput improved ~8%.

3) TVL : --rate 350000 --spouts 1 --splitters 2 --counters 2 -c topology.max.spout.pending=500 -c topology.acker.executors=2 -c topology.disable.loadaware.messaging=true

No significant change observed in Throughput or Latency.

roshannaik · 2018-06-15T09:59:21Z

Looks like tester_bolt_metrics.py under storm-core is failing. I see that it can be run via mvn clojure:test under storm-core. But unable to figure out what really going wrong. Any help would be appreciated. thanks.

HeartSaVioR · 2018-06-19T09:07:07Z

@roshannaik
It looks like Zk issue but not 100% sure. If it is happening intermittently we might be fine. Could you setup Travis to your fork and run build couple of times?

revans2

Overall it looks good. I have a number of nits/questions Mostly about the tests.

revans2 · 2018-06-20T15:58:18Z

storm-client/src/jvm/org/apache/storm/utils/CustomIndexArray.java

+        Integer lowerIndex = Integer.MAX_VALUE;
+        Integer upperIndex = Integer.MIN_VALUE;
+
+        for (Map.Entry<Integer, T> entry : src.entrySet()) { // calculate smallest & largest indexes


nit: would src.keySet() be simpler because we don't use the values?

revans2 · 2018-06-20T15:59:37Z

storm-client/src/jvm/org/apache/storm/utils/CustomIndexArray.java

+
+        this.elements = makeNullInitializedArray(elemCount);
+
+        for (Map.Entry<Integer, T> entry : src.entrySet()) {


nit: would calling set improve code reuse a little?

Yes, calling set() directly instead of elements.set() would have been nice..But normally I dont call instance methods from constructors since the object is not considered as fully initialized until the constructor has finished.

revans2 · 2018-06-20T16:00:26Z

storm-client/src/jvm/org/apache/storm/utils/CustomIndexArray.java

+    /**
+     * Always returns true as this cannot be empty.
+     */
+    public boolean isEmpty() {


Why have this at all. It does not implement collection so we don't need it.

revans2 · 2018-06-20T16:04:57Z

storm-client/src/jvm/org/apache/storm/utils/CustomIndexArray.java

+    /**
+     * Returns the index range as a Set.
+     */
+    public Set<Integer> indices() {


Nit: we are keeping the order of the indices here, but it is not documented in the javadocs, and it is not needed, as the only place this is used is for doing a set difference.

It would also be nice to indicate that this is not a cheap call so people don't abuse it calling it a lot.

sue.. I just felt its intuitive and nice to maintain the integers in order even if not absolutely necessary. Updating the javadoc.

revans2 · 2018-06-20T16:06:50Z

...rnal/storm-elasticsearch/src/test/java/org/apache/storm/elasticsearch/common/EsTestUtil.java

@@ -63,7 +64,7 @@
    public static Tuple generateTestTuple(String source, String index, String type, String id) {
        TopologyBuilder builder = new TopologyBuilder();
        GeneralTopologyContext topologyContext = new GeneralTopologyContext(builder.createTopology(),
-                new Config(), new HashMap<>(), new HashMap<>(), new HashMap<>(), "") {
+                new Config(),  new CustomIndexArray<String>(0,1), new HashMap<>(), new HashMap<>(), "") {


nit: extra space.

revans2 · 2018-06-20T16:08:05Z

...m-elasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/common/EsTestUtil.java

@@ -56,7 +57,7 @@ public static Tuple generateTestTuple(final String source,
        GeneralTopologyContext topologyContext = new GeneralTopologyContext(
                builder.createTopology(),
                new Config(),
-                new HashMap<>(),
+                new CustomIndexArray<String>(0,1),


Nit: java should be able to figure out that it is a String from the required type for GeneralTopologyContext

It is mostly for readability .. when reading this code its unclear what the deduced type will be without further checking the parameter types for the constructor. Happy to change if you prefer the other way.

revans2 · 2018-06-20T16:08:31Z

...m-elasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/common/EsTestUtil.java

@@ -56,7 +57,7 @@ public static Tuple generateTestTuple(final String source,
        GeneralTopologyContext topologyContext = new GeneralTopologyContext(
                builder.createTopology(),
                new Config(),
-                new HashMap<>(),
+                new CustomIndexArray<String>(0,1),


Also why does this need to be 0,1?

Just any non empty range. will change it to (0,0)

revans2 · 2018-06-20T16:09:21Z

...nal/storm-hdfs/src/test/java/org/apache/storm/hdfs/bolt/format/TestSimpleFileNameFormat.java

@@ -69,8 +70,8 @@ public void testTimeFormat() {
    }

    private TopologyContext createTopologyContext(Map<String, Object> topoConf) {
-        Map<Integer, String> taskToComponent = new HashMap<Integer, String>();
-        taskToComponent.put(7, "Xcom");
+        CustomIndexArray<String> taskToComponent = new CustomIndexArray<>(0,8);


Why 0,8 if all we are setting is 7?

revans2 · 2018-06-20T16:18:26Z

storm-client/src/jvm/org/apache/storm/daemon/Task.java

@@ -112,15 +112,15 @@ public Task(Executor executor, Integer taskId) throws IOException {
    public List<Integer> getOutgoingTasks(Integer outTaskId, String stream, List<Object> values) {
        if (debug) {
            LOG.info("Emitting direct: {}; {} {} {} ", outTaskId, componentId, stream, values);
-        }


I think grouping check should happen even if debug is not enabled.

That was part of an older PR. From what I recall ... on profiling I had noticed that the grouping check was expensive in the critical path due to the fact that it needed lookups in three (now down to 2) hashmaps : streamComponentToGrouper & componentGrouping. Since neither were keyed on Integer, the CustomIndexArray style optimization was not possible.

revans2 · 2018-06-20T16:24:27Z

storm-server/src/main/java/org/apache/storm/Testing.java

@@ -660,8 +661,8 @@ public static Tuple testTuple(List<Object> values, MkTupleParam param) {
            }
        }

-        Map<Integer, String> taskToComp = new HashMap<>();
-        taskToComp.put(task, component);
+        CustomIndexArray<String> taskToComp = new CustomIndexArray<>(task,task+1);


If lower and upper are inclusive why do we need task, task+1?

roshannaik · 2018-07-02T10:46:16Z

Sorry for not being able to address the comments sooner. Have been traveling and should be able to get back to this soon after jul 12 th.

HeartSaVioR · 2018-07-16T21:25:30Z

@roshannaik

https://travis-ci.org/apache/storm/jobs/403972342
https://travis-ci.org/apache/storm/jobs/403972347
https://travis-ci.org/apache/storm/jobs/403972348
https://travis-ci.org/apache/storm/jobs/403972349

Travis CI build is failing and failures look like related to the changeset. Could you check above build result and track?

…tId & ExecutorTransfer.localReceiveQueues

HeartSaVioR · 2018-07-29T06:57:38Z

Looks like the build still fails and relevant to this patch.

roshannaik · 2018-08-03T02:40:04Z

Spent sometime trying to triage this failure but unable to figure out the cause. i know the issue has to do with this patch as it passes w/o this patch. I think it will take me some more time to get to the bottom of this. I did notice another (kind of significant) performance issue which is simple to address. Given that this is a relatively minor optimization, I am planning to provide a fix for that one first, and then revert back to this.

roshannaik force-pushed the STORM-3100 branch 2 times, most recently from bc9cbb7 to 9d875a0 Compare June 14, 2018 09:14

revans2 approved these changes Jun 20, 2018

View reviewed changes

roshannaik force-pushed the STORM-3100 branch from 9d875a0 to 6f311c8 Compare July 14, 2018 19:23

STORM-3100 : Introducing CustomIndexArray. Using it for task2componen…

66d06c8

…tId & ExecutorTransfer.localReceiveQueues

roshannaik force-pushed the STORM-3100 branch from 6f311c8 to 66d06c8 Compare July 23, 2018 05:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STORM-3100 : Introducing CustomIndexArray to speedup HashMap lookups #2711

STORM-3100 : Introducing CustomIndexArray to speedup HashMap lookups #2711

roshannaik commented Jun 10, 2018 •

edited

roshannaik commented Jun 15, 2018

HeartSaVioR commented Jun 19, 2018 •

edited

revans2 left a comment

revans2 Jun 20, 2018

revans2 Jun 20, 2018

roshannaik Jun 23, 2018

revans2 Jun 20, 2018

revans2 Jun 20, 2018

roshannaik Jun 23, 2018 •

edited

revans2 Jun 20, 2018

revans2 Jun 20, 2018

roshannaik Jun 23, 2018

revans2 Jun 20, 2018

roshannaik Jun 23, 2018 •

edited

revans2 Jun 20, 2018

revans2 Jun 20, 2018

roshannaik Jun 23, 2018

revans2 Jun 20, 2018

roshannaik commented Jul 2, 2018

HeartSaVioR commented Jul 16, 2018

HeartSaVioR commented Jul 29, 2018

roshannaik commented Aug 3, 2018 •

edited


		this.elements = makeNullInitializedArray(elemCount);

		for (Map.Entry<Integer, T> entry : src.entrySet()) {

STORM-3100 : Introducing CustomIndexArray to speedup HashMap lookups #2711

Are you sure you want to change the base?

STORM-3100 : Introducing CustomIndexArray to speedup HashMap lookups #2711

Conversation

roshannaik commented Jun 10, 2018 • edited

1) ConstSpoutIdBoltNullBoltTopo With 1 Acker

2) ConstSpoutIdBoltNullBoltTopo With 0 Acker

3) TVL : --rate 350000 --spouts 1 --splitters 2 --counters 2 -c topology.max.spout.pending=500 -c topology.acker.executors=2 -c topology.disable.loadaware.messaging=true

roshannaik commented Jun 15, 2018

HeartSaVioR commented Jun 19, 2018 • edited

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roshannaik Jun 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roshannaik Jun 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roshannaik commented Jul 2, 2018

HeartSaVioR commented Jul 16, 2018

HeartSaVioR commented Jul 29, 2018

roshannaik commented Aug 3, 2018 • edited

roshannaik commented Jun 10, 2018 •

edited

HeartSaVioR commented Jun 19, 2018 •

edited

roshannaik Jun 23, 2018 •

edited

roshannaik Jun 23, 2018 •

edited

roshannaik commented Aug 3, 2018 •

edited