SOLR-15428: Integrate the OpenJDK JMH micro benchmark framework for m… #214

markrmiller · 2021-07-13T15:26:32Z

Draft stripped down PR

solr/test-framework/src/java/org/apache/solr/util/RandomizeSSL.java

madrob · 2021-07-14T16:58:54Z

Can you add a task for gradlew helpJmh describing the various flags and usages that developers should consider? Should be straightforward as adding a line to help.gradle and a new file under help/ directory.

dsmiley

I did a review but have yet to try this, which I am anxious to do.

It seems there is an overarching issue relating to the benchmark running classes in test-framework yet not having a RandomizedTesting unit test runner and thus no random seed? Let's ping Dawid about this use-case; ehh? It'd be nice to explicitly initialize that framework without using the runner.

solr/test-framework/src/java/org/apache/solr/util/RandomizeSSL.java

solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java

solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java

solr/test-framework/build.gradle

solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java

solr/test-framework/src/jmh/org/apache/solr/bench/DocMakerRamGen.java

solr/test-framework/src/jmh/org/apache/solr/bench/FieldDef.java

dsmiley

Was this capability embedded into the test-framework module instead of being a new module because we want to publish the foundation so other modules (including 3rd party) can use them? test-framework is about publishing utilities for other modules, so that's the only reason I can think of why this was added there. Otherwise, it seems a new module would be more appropriate.

markrmiller · 2021-07-19T17:34:03Z

bq. Otherwise, it seems a new module would be more appropriate.

You don't back up that statement with any support, other than:

bq. test-framework is about publishing utilities for other modules

Which I'm not sure I agree with.

Initially, I will say its here for:

initial ease
overlap in code/use/idea - often a benchmark will share much with how it's unit test is created, use some of the same supporting classes. The unit tests help test correctness, the jmh benchmarks help test performance. The test-framwork modules has support / base classes for 'testing' (correctness, performance, etc).

I like to stack up reasons for going one way or another, but I see the test-framework module as the home of the testing framework, not "about publishing utilities for other modules", so not really adding to my calculus in that form.

I also thought there would be more that was shared initially though. And I've sense realized this could use more separation from the randomized testing framework layers and requirements, and the different needs do make reuse of items between benchmark and junit tests a bit less simple and with more of a need to make sure mini clusters and jettyrunners are happy outside of randomized runner stuff.

solr/benchmark/build.gradle

dsmiley

I really like the move to its own module! It'll be nice to see them all easily as we learn to explore this capability -- it's more discoverable. It also appears to fit in better from a build/Gradle perspective than when it was embedded in solr-test-framework.

madrob · 2021-07-21T16:55:05Z

solr/benchmark/src/java/org/apache/solr/bench/DocMakerRamGen.java

+    Validate.isTrue(endExclusive >= startInclusive,
+        "Start value must be smaller or equal to end value.");
+    Validate.isTrue(startInclusive >= 0, "Both range values must be non-negative.");


Can use Objects.checkIndex for this

madrob · 2021-07-21T16:55:44Z

solr/benchmark/build.gradle

+  args '-jvmArgsPrepend', '-Dlog4j2.is.webapp=false'
+  args '-jvmArgsPrepend', '-Dlog4j2.garbagefreeThreadContextMap=true'
+  args '-jvmArgsPrepend', '-Dlog4j2.enableDirectEncoders=true'
+  args '-jvmArgsPrepend', '-Dlog4j2.enable.threadlocals=true'


Can these make their way into our standard deployment as well?

https://issues.apache.org/jira/browse/SOLR-15477 Coming soon.

sonatype-lift · 2021-07-29T18:18:57Z

solr/benchmark/jmh.sh

+
+echo "gradle build done"
+
+echo "running JMH with args: $@"


ShellCheck: Argument mixes string and array. Use * or separate argument.
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:51Z

solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java

+
+      // dump Solr metrics
+      Path metricsResults =
+          Paths.get(


PATH_TRAVERSAL_IN: This API (java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path;) reads a file whose location might be specified by user input (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:52Z

solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java

+      Set<String> registryNames = metricsManager.registryNames();
+      for (String registryName : registryNames) {
+        MetricRegistry metricsRegisty = metricsManager.registry(registryName);
+        try (PrintStream ps = outputDirectory == null ? new NullPrintStream() : new PrintStream(new File(outputDirectory,  registryName + "_" + fileName), StandardCharsets.UTF_8)) {


PATH_TRAVERSAL_IN: This API (java/io/File.(Ljava/io/File;Ljava/lang/String;)V) reads a file whose location might be specified by user input (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:53Z

solr/benchmark/src/java/org/apache/solr/bench/DocMaker.java

+        SplittableRandom random = new SplittableRandom(seed);
+        if (fieldDef.getLength() > -1) {
+          return RandomStringUtils.random(
+              nextInt(fieldDef.getLength(), fieldDef.getLength(), random),


PREDICTABLE_RANDOM: This random generator (java.util.Random) is predictable (details)
(at-me in a reply with help or ignore)

sonatype-lift · 2021-08-05T02:04:54Z

solr/benchmark/src/java/org/apache/solr/bench/DocMaker.java

+            ThreadLocalRandom.current(), fieldDef.getLength(), fieldDef.getLength());
+      } else {
+        return TestUtil.randomRealisticUnicodeString(
+            ThreadLocalRandom.current(), 1, fieldDef.getMaxLength());


PREDICTABLE_RANDOM: This random generator (java.util.concurrent.ThreadLocalRandom) is predictable (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:56Z

solr/benchmark/src/java/org/apache/solr/bench/FieldDef.java

+      }
+      this.maxCardinality = maxCardinality;
+      this.cardinalityStart =
+          ThreadLocalRandom.current().nextLong(0, Long.MAX_VALUE - maxCardinality);


PREDICTABLE_RANDOM: This random generator (java.util.concurrent.ThreadLocalRandom) is predictable (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:57Z

solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java

+      }
+
+      // set the seed used by ThreadLocalRandom
+      System.setProperty("randomSeed", Long.toString(new Random(seed).nextLong()));


PREDICTABLE_RANDOM: This random generator (java.util.Random) is predictable (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:58Z

solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java

+
+      QueryRequest queryRequest = new QueryRequest(new SolrQuery("q", "*:*", "rows", "1"));
+      queryRequest.setBasePath(
+          nodes.get(ThreadLocalRandom.current().nextInt(cluster.getJettySolrRunners().size())));


PREDICTABLE_RANDOM: This random generator (java.util.concurrent.ThreadLocalRandom) is predictable (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:04:59Z

solr/benchmark/src/java/org/apache/solr/bench/index/CloudIndexing.java

+      throws Exception {
+    UpdateRequest updateRequest = new UpdateRequest();
+    updateRequest.setBasePath(
+        miniClusterState.nodes.get(ThreadLocalRandom.current().nextInt(state.nodeCount)));


PREDICTABLE_RANDOM: This random generator (java.util.concurrent.ThreadLocalRandom) is predictable (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:05:01Z

solr/benchmark/src/java/org/apache/solr/bench/search/JsonFaceting.java

+      throws Exception {
+    QueryRequest queryRequest = new QueryRequest(state.params);
+    queryRequest.setBasePath(
+        miniClusterState.nodes.get(ThreadLocalRandom.current().nextInt(state.nodeCount)));


PREDICTABLE_RANDOM: This random generator (java.util.concurrent.ThreadLocalRandom) is predictable (details)
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:05:03Z

solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java

+        }
+      }
+
+      SolrMetricManager metricsManager = getCoreContainer().getMetricManager();


NULL_DEREFERENCE: object returned by getCoreContainer() could be null and is dereferenced at line 729.
(at-me in a reply with help or ignore)

sonatype-lift · 2021-08-05T02:05:06Z

solr/core/src/java/org/apache/solr/core/SolrCore.java

@@ -487,6 +488,14 @@ public long getIndexSize() {
    return size;
  }

+  public int getSegmentCount() {
+    try {
+      return withSearcher( solrIndexSearcher -> solrIndexSearcher.getRawReader().getIndexCommit().getSegmentCount());


THREAD_SAFETY_VIOLATION: Read/Write race. Non-private method SolrCore.getSegmentCount() indirectly reads with synchronization from this.indexReaderFactory. Potentially races with unsynchronized write in method SolrCore.initIndex(...).
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:05:10Z

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java

@@ -212,7 +197,7 @@ public MiniSolrCloudCluster(int numServers, String hostContext, Path baseDir, St
   * @throws Exception if there was an error starting the cluster
   */
  public MiniSolrCloudCluster(int numServers, Path baseDir, String solrXml, JettyConfig jettyConfig) throws Exception {
-    this(numServers, baseDir, solrXml, jettyConfig, null);
+    this(numServers, baseDir, solrXml, jettyConfig, null, false);


THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method MiniSolrCloudCluster(...) indirectly mutates container util.ObjectReleaseTracker.OBJECTS via call to Map.remove(...) outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:05:11Z

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java

-      ZkTestServer zkTestServer) throws Exception {
-    this(numServers, baseDir, solrXml, jettyConfig, zkTestServer, Optional.empty());
+      ZkTestServer zkTestServer, boolean formatZkServer) throws Exception {
+    this(numServers, baseDir, solrXml, jettyConfig, zkTestServer, Optional.empty(), formatZkServer);


THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method MiniSolrCloudCluster(...) indirectly mutates container util.ObjectReleaseTracker.OBJECTS via call to Map.remove(...) outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:05:12Z

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java

+        }
+        if (securityJson.isPresent()) { // configure Solr security
+          zkClient.makePath("/solr/security.json", securityJson.get().getBytes(Charset.defaultCharset()), true);
+        }
      }
    }


THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method MiniSolrCloudCluster(...) indirectly mutates container util.ObjectReleaseTracker.OBJECTS via call to Map.remove(...) outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me in a reply with help or ignore)

@sonatype-lift ignore

sonatype-lift · 2021-08-05T02:05:14Z

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java

+    AtomicReference<DocCollection> state = new AtomicReference<>();
+    AtomicReference<Set<String>> liveNodesLastSeen = new AtomicReference<>();
+    try {
+      getSolrClient().waitForState(collection, wait, unit, (n, c) -> {


THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method MiniSolrCloudCluster.waitForActiveCollection(...) indirectly writes to field noggit.JSONParser.devNull.buf outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me in a reply with help or ignore)

@sonatype-lift ignore

madrob · 2021-08-05T20:54:00Z

Do we need to include a JFR or async-profiler recording profile?

markrmiller · 2021-08-09T19:52:08Z

Do we need to include a JFR or async-profiler recording profile?

There is a sample config for JFR (that essentially turns on everything), but in general, these options are configured on the command line depending on what you are trying to look at.

…icro benchmarks and performance comparisons & investigation.

…icro benchmarks and performance comparisons & investigation. (apache#214)

markrmiller force-pushed the SOLR-15428-CORE branch 2 times, most recently from fd9a36a to 16b22d2 Compare July 13, 2021 16:31

sonatype-lift bot reviewed Jul 13, 2021

View reviewed changes

solr/test-framework/src/java/org/apache/solr/util/RandomizeSSL.java Outdated Show resolved Hide resolved

markrmiller force-pushed the SOLR-15428-CORE branch 2 times, most recently from 611102e to 9297745 Compare July 14, 2021 16:18

markrmiller force-pushed the SOLR-15428-CORE branch 5 times, most recently from 6b7735b to 24b086f Compare July 15, 2021 00:07

dsmiley requested changes Jul 17, 2021

View reviewed changes

dsmiley reviewed Jul 18, 2021

View reviewed changes

markrmiller force-pushed the SOLR-15428-CORE branch from 24b086f to 18f041f Compare July 20, 2021 00:52

sonatype-lift bot reviewed Jul 20, 2021

View reviewed changes

solr/benchmark/build.gradle Show resolved Hide resolved

dsmiley reviewed Jul 20, 2021

View reviewed changes

madrob reviewed Jul 21, 2021

View reviewed changes

markrmiller force-pushed the SOLR-15428-CORE branch 11 times, most recently from 1ba7f6c to eb9339d Compare July 29, 2021 17:22

sonatype-lift bot reviewed Jul 29, 2021

View reviewed changes

markrmiller force-pushed the SOLR-15428-CORE branch 3 times, most recently from 0e96672 to d4974fd Compare August 5, 2021 01:09

sonatype-lift bot reviewed Aug 5, 2021

View reviewed changes

markrmiller force-pushed the SOLR-15428-CORE branch 2 times, most recently from eab2742 to 000e83a Compare August 5, 2021 16:46

markrmiller force-pushed the SOLR-15428-CORE branch 3 times, most recently from 48570ed to 9e6fa16 Compare August 9, 2021 17:54

SOLR-15428: Integrate the OpenJDK JMH micro benchmark framework for m…

11ec45f

…icro benchmarks and performance comparisons & investigation.

markrmiller force-pushed the SOLR-15428-CORE branch from 9e6fa16 to 11ec45f Compare August 9, 2021 19:55

markrmiller merged commit 890ef78 into apache:main Aug 10, 2021

epugh pushed a commit to epugh/solr that referenced this pull request Oct 22, 2021

SOLR-15428: Integrate the OpenJDK JMH micro benchmark framework for m…

5be8228

…icro benchmarks and performance comparisons & investigation. (apache#214)

SOLR-15428: Integrate the OpenJDK JMH micro benchmark framework for m… #214

SOLR-15428: Integrate the OpenJDK JMH micro benchmark framework for m… #214

Conversation

markrmiller commented Jul 13, 2021

madrob commented Jul 14, 2021

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

markrmiller commented Jul 19, 2021

dsmiley 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

sonatype-lift bot Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

markrmiller Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madrob commented Aug 5, 2021

markrmiller commented Aug 9, 2021

markrmiller Aug 5, 2021 •

edited

Loading