Use map.putIfAbsent() or map.computeIfAbsent() as appropriate instead of containsKey() + put() #7764

sashidhar · 2019-05-26T11:54:45Z

if(!map.containsKey(some_key)) {
key.put(some_key, some_value);
}
The above map usage pattern has been replaced with map.putIfAbsent()
A new search configuration got updated in .idea/inspectionProfiles/Druid.xml.

Merging changes from master branch of apache/incubator-druid repo

sashidhar · 2019-05-26T12:09:08Z

Checking the failures.

gianm · 2019-05-26T19:52:21Z

The test error looks potentially legitimate; please check this test and any others referenced in the Travis CI report:

org.apache.druid.indexing.firehose.IngestSegmentFirehoseFactoryTest.testGetUniqueDimensionsAndMetrics[DimNames[dims]MetricNames[metrics]ParserDimNames[dims]WrapInCombining[false]](org.apache.druid.indexing.firehose.IngestSegmentFirehoseFactoryTest)
  Run 1: IngestSegmentFirehoseFactoryTest.testGetUniqueDimensionsAndMetrics:539 expected:<[met9, met10, met11, met12, met13, met8, met7, met6, met5, met4, met3, met2, met1, met0]> but was:<[met9, met10, met11, met12, met13, met8, null, null, null, null, met7, null, null, null]>

gianm · 2019-05-26T19:54:58Z

.idea/inspectionProfiles/Druid.xml

@@ -306,6 +303,11 @@
        <constraint name="x" maxCount="2147483647" within="" contains="" />
        <constraint name="ImmutableMap" regexp="Immutable.*" within="" contains="" />
      </searchConfiguration>
+      <searchConfiguration name="Use Map.putIfAbsent() instead of containsKey() + put()" created="1558868694225" text="if (!$m$.containsKey($k$)) {&#10;    $m$.put($k$, $v$);&#10;}" recursive="false" caseInsensitive="true" type="JAVA">


This isn't always good advice: in particular, if v is expensive to compute then computeIfAbsent should be used instead of putIfAbsent. This is because putIfAbsent computes v eagerly, whereas containsKey + put (or computeIfAbsent) computes v lazily. This can also have a deeper effect on behavior, if computation of v has side effects.

After reviewing the rest of the patch, it seems like computeIfAbsent is appropriate more often than putIfAbsent, so the recommendation here should reflect that.

Sure @gianm , I'll make the necessary changes.

I've made the changes. Some of the changes are straight forward. I'll comment on those respective changes which are notable.

gianm · 2019-05-26T19:58:49Z

indexing-hadoop/src/main/java/org/apache/druid/indexer/DetermineHashedPartitionsJob.java

-        if (!hyperLogLogs.containsKey(interval)) {
-          hyperLogLogs.put(interval, HyperLogLogCollector.makeLatestCollector());
-        }
+        hyperLogLogs.putIfAbsent(interval, HyperLogLogCollector.makeLatestCollector());


This is a good candidate for computeIfAbsent.

gianm · 2019-05-26T19:59:36Z

...vice/src/main/java/org/apache/druid/indexing/appenderator/ActionBasedUsedSegmentChecker.java

-      if (!identifiersByDataSource.containsKey(identifier.getDataSource())) {
-        identifiersByDataSource.put(identifier.getDataSource(), new HashSet<>());
-      }
+      identifiersByDataSource.putIfAbsent(identifier.getDataSource(), new HashSet<>());


This is a good candidate for computeIfAbsent.

gianm · 2019-05-26T19:59:48Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java

-            if (!hllCollectors.containsKey(interval)) {
-              hllCollectors.put(interval, Optional.of(HyperLogLogCollector.makeLatestCollector()));
-            }
+            hllCollectors.putIfAbsent(interval, Optional.of(HyperLogLogCollector.makeLatestCollector()));


This is a good candidate for computeIfAbsent.

gianm · 2019-05-26T20:00:43Z

...g-service/src/main/java/org/apache/druid/indexing/firehose/IngestSegmentFirehoseFactory.java

-          if (!segmentFileMap.containsKey(segment)) {
-            segmentFileMap.put(segment, segmentLoader.getSegmentFiles(segment));
-          }
+          segmentFileMap.putIfAbsent(segment, segmentLoader.getSegmentFiles(segment));


This needs to be computeIfAbsent, since there are side effects.

gianm · 2019-05-26T20:02:39Z

server/src/test/java/org/apache/druid/client/CachingClusteredClientTest.java

-        if (!serverExpectations.containsKey(lastServer)) {
-          serverExpectations.put(lastServer, new ServerExpectations(lastServer, makeMock(mocks, QueryRunner.class)));
-        }
+        serverExpectations.putIfAbsent(lastServer, new ServerExpectations(lastServer, makeMock(mocks, QueryRunner.class)));


Another good candidate for computeIfAbsent.

gianm · 2019-05-26T20:03:05Z

sql/src/main/java/org/apache/druid/sql/SqlLifecycle.java

-    if (!newContext.containsKey(PlannerContext.CTX_SQL_QUERY_ID)) {
-      newContext.put(PlannerContext.CTX_SQL_QUERY_ID, UUID.randomUUID().toString());
-    }
+    newContext.putIfAbsent(PlannerContext.CTX_SQL_QUERY_ID, UUID.randomUUID().toString());


Another good candidate for computeIfAbsent.

gianm · 2019-05-26T20:03:15Z

sql/src/test/java/org/apache/druid/sql/calcite/util/SpecificSegmentsQuerySegmentWalker.java

-    if (!timelines.containsKey(descriptor.getDataSource())) {
-      timelines.put(descriptor.getDataSource(), new VersionedIntervalTimeline<>(Ordering.natural()));
-    }
+    timelines.putIfAbsent(descriptor.getDataSource(), new VersionedIntervalTimeline<>(Ordering.natural()));


Another good candidate for computeIfAbsent.

gianm · 2019-05-26T20:05:58Z

.idea/inspectionProfiles/Druid.xml

@@ -88,9 +88,6 @@
    <inspection_tool class="JSRedeclarationOfBlockScope" enabled="true" level="Non-TeamCity Error" enabled_by_default="true" />
    <inspection_tool class="JSReferencingArgumentsOutsideOfFunction" enabled="true" level="Non-TeamCity Error" enabled_by_default="true" />
    <inspection_tool class="Java8MapForEach" enabled="true" level="ERROR" enabled_by_default="true" />
-    <inspection_tool class="JavadocReference" enabled="true" level="ERROR" enabled_by_default="true">


Why did this change?

Reverted this change.

gianm · 2019-05-26T20:06:04Z

.idea/inspectionProfiles/Druid.xml

@@ -161,7 +158,7 @@
      <searchConfiguration name="Suboptimal IndexedInts iteration" text="$x$ &lt; $y$.size()" recursive="false" caseInsensitive="true" type="JAVA">
        <constraint name="__context__" target="true" within="" contains="" />
        <constraint name="x" within="" contains="" />
-        <constraint name="y" nameOfExprType="IndexedInts" exprTypeWithinHierarchy="true" within="" contains="" />
+        <constraint name="y" nameOfExprType="IndexedInts" expressionTypes="IndexedInts" exprTypeWithinHierarchy="true" within="" contains="" />


Why did this change?

When using different versions of IntelliJ IDEA, it makes some seemingly arbitrary changes to the config by itself. As long as it doesn't break the TeamCity build (and if the PR author uses the latest version of IntelliJ), it should be fine.

@leventov, thanks for the info. I've reverted the changes auto made by the IDE.

…riate

sashidhar · 2019-05-27T19:12:25Z

...g-service/src/main/java/org/apache/druid/indexing/firehose/IngestSegmentFirehoseFactory.java

+
+        segmentFileMap.computeIfAbsent(segment, k -> {
+          try {
+            return segmentLoader.getSegmentFiles(segment);


segmentLoader.getSegmentFiles(segment) throws SegmentLoadingException, which needs to be caught and re thrown as RuntimeException. This makes the outer try catch redundant (compilation error) as the SegmentLoadingException has already been caught.

sashidhar · 2019-05-27T19:12:57Z

...g-service/src/main/java/org/apache/druid/indexing/firehose/IngestSegmentFirehoseFactory.java

-    }
-    catch (SegmentLoadingException e) {
-      throw new RuntimeException(e);
-    }


Outer try/catch block removed.

sashidhar · 2019-05-27T19:15:03Z

...g-service/src/main/java/org/apache/druid/indexing/firehose/IngestSegmentFirehoseFactory.java

-          if (!uniqueMetrics.containsKey(metric)) {
-            uniqueMetrics.put(metric, index++);
+          uniqueMetrics.computeIfAbsent(metric, k -> {
+            return index[0]++;
          }


Using index variable as int as is, the compiler complains "Variables used in lambda should be final or effectively final". The fix is to use an integer array with one element. Let me know if this is right.

sashidhar · 2019-05-28T06:48:31Z

@gianm , @leventov
This is the failure. Can you help me understand this recommendation.

Selected: Structural Search Inspection (1)
indexing-service/src/main/java/org/apache/druid/indexing/overlord
ForkingTaskRunner.java (1)
217:  A ConcurrentHashMap on which computeIfAbsent() is called should be assigned into variables of ConcurrentHashMap type, not ConcurrentMap

This is the line. Why should ConcurrentMap changed to ConcurrentHashMap type here ?
private final ConcurrentMap<String, ForkingTaskRunnerWorkItem> tasks = new ConcurrentHashMap<>();

clintropolis · 2019-05-28T07:29:49Z

This is the line. Why should ConcurrentMap changed to ConcurrentHashMap type here ?

This check was added in #6898, I think maybe this comment is relevant to what it is trying to enforce.

…sent() is called should be assigned into variables of ConcurrentHashMap type, not ConcurrentMap

sashidhar · 2019-05-28T16:24:23Z

Thanks @clintropolis this helps.

leventov · 2019-05-28T17:01:43Z

@clintropolis the reference place with the explanation is now here: https://github.com/code-review-checklists/java-concurrency#chm-type

sashidhar · 2019-05-29T02:48:03Z

@gianm, @leventov All checks have passed. Please review.

sashidhar · 2019-05-30T17:36:15Z

@gianm, @leventov Gentle reminder.

gianm

LGTM!

sashidhar · 2019-06-14T10:15:56Z

@leventov Please review.

leventov · 2019-06-14T15:59:22Z

The PR doesn't require my review because it has one approval from a committer and it's not tagged Design Review.

sashidhar · 2019-06-14T17:36:49Z

Thanks @leventov.

sashidhar and others added 2 commits May 26, 2019 16:27

Merge pull request #1 from apache/master

37ebf6e

Merging changes from master branch of apache/incubator-druid repo

apache#7316 Use Map.putIfAbsent() instead of containsKey() + put()

50b80a0

sashidhar mentioned this pull request May 26, 2019

Prohibit a Map usage pattern replaceable with putIfAbsent() #7316

Closed

fixing indentation

b8e4473

gianm reviewed May 26, 2019

View reviewed changes

t-sashidhar added 6 commits May 27, 2019 23:52

Using map.computeIfAbsent() instead of map.putIfAbsent() where approp…

b54c36e

…riate

Merge branch 'master' of https://github.com/apache/incubator-druid

2b6cf49

Merge branch 'master' into map_usage_pattern

a3f6019

fixing checkstyle

0d90c3b

Changing the recommendation text

2eb5c81

Reverting auto changes made by IDE

71a6ce0

sashidhar commented May 27, 2019

View reviewed changes

t-sashidhar added 2 commits May 28, 2019 16:13

Implementing recommendation: A ConcurrentHashMap on which computeIfAb…

0cee976

…sent() is called should be assigned into variables of ConcurrentHashMap type, not ConcurrentMap

Removing unused import

e3ebf0d

sashidhar changed the title ~~#7316 Use Map.putIfAbsent() instead of containsKey() + put()~~ #7316 Use map.putIfAbsent() or map.computeIfAbsent() as appropriate instead of containsKey() + put() May 28, 2019

gianm approved these changes Jun 6, 2019

View reviewed changes

leventov added the Area - Automation/Static Analysis label Jun 14, 2019

leventov changed the title ~~#7316 Use map.putIfAbsent() or map.computeIfAbsent() as appropriate instead of containsKey() + put()~~ Use map.putIfAbsent() or map.computeIfAbsent() as appropriate instead of containsKey() + put() Jun 14, 2019

leventov merged commit 3bee6ad into apache:master Jun 14, 2019

sashidhar deleted the map_usage_pattern branch July 30, 2019 19:37

clintropolis added this to the 0.16.0 milestone Aug 8, 2019

Use map.putIfAbsent() or map.computeIfAbsent() as appropriate instead of containsKey() + put() #7764

Use map.putIfAbsent() or map.computeIfAbsent() as appropriate instead of containsKey() + put() #7764

Conversation

sashidhar commented May 26, 2019 • edited by leventov Loading

sashidhar commented May 26, 2019

gianm commented May 26, 2019

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

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

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

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

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

sashidhar commented May 28, 2019 • edited Loading

clintropolis commented May 28, 2019

sashidhar commented May 28, 2019

leventov commented May 28, 2019

sashidhar commented May 29, 2019

sashidhar commented May 30, 2019

gianm left a comment

Choose a reason for hiding this comment

sashidhar commented Jun 14, 2019

leventov commented Jun 14, 2019

sashidhar commented Jun 14, 2019

sashidhar commented May 26, 2019 •

edited by leventov

Loading

sashidhar commented May 28, 2019 •

edited

Loading