You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/09/12 23:09:02 UTC

[GitHub] [lucene] markrmiller opened a new pull request #295: LUCENE-10099: Add -Ptests.asyncprofile option.

markrmiller opened a new pull request #295:
URL: https://github.com/apache/lucene/pull/295


   Here is a minimal rough draft for a tests.asyncprofile option that uses the java async profiler: https://github.com/jvm-profiling-tools/async-profiler
   
   Still needs a variety of configuration options at a minimum, some proper doc, maybe something else. This demonstrates a working example though.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org


[GitHub] [lucene] rmuir commented on pull request #295: LUCENE-10099: Add -Ptests.asyncprofile option.

Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #295:
URL: https://github.com/apache/lucene/pull/295#issuecomment-917803069


   If the user has java 16 or above, and we pass `-XX:+UnlockDiagnosticVMOptions -XX:+DumpPerfMapAtExit -XX:+PreserveFramePointer` to the JVM, we should get java symbol resolution from `perf` subsystem? Will it help this profiler? I think we can just add these args guarded by `if (rootProject.runtimeJavaVersion >= JavaVersion.VERSION_16) {` so that older JVMs still work tool. See https://bugs.openjdk.java.net/browse/JDK-8254723


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org


[GitHub] [lucene] markrmiller commented on a change in pull request #295: LUCENE-10099: Add -Ptests.asyncprofile option.

Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #295:
URL: https://github.com/apache/lucene/pull/295#discussion_r706926916



##########
File path: buildSrc/src/main/java/org/apache/lucene/gradle/ProfileResults.java
##########
@@ -156,42 +156,42 @@ public static void printReport(List<String> files, String mode, int stacksize, i
     if (!"cpu".equals(mode) && !"heap".equals(mode)) {
       throw new IllegalArgumentException("tests.profile.mode must be one of (cpu,heap)");
     }
-    if (stacksize < 1) {
-      throw new IllegalArgumentException("tests.profile.stacksize must be positive");
-    }
-    if (count < 1) {
-      throw new IllegalArgumentException("tests.profile.count must be positive");
-    }
-    Map<String, SimpleEntry<String, Long>> histogram = new HashMap<>();
-    int totalEvents = 0;
-    long sumValues = 0;
-    String framePadding = " ".repeat(COLUMN_SIZE * 2);
-    for (String file : files) {
-      try (RecordingFile recording = new RecordingFile(Paths.get(file))) {
-        while (recording.hasMoreEvents()) {
-          RecordedEvent event = recording.readEvent();
-          if (!isInteresting(mode, event)) {
-            continue;
-          }
-          RecordedStackTrace trace = event.getStackTrace();
-          if (trace != null) {
-            StringBuilder stack = new StringBuilder();
-            for (int i = 0; i < Math.min(stacksize, trace.getFrames().size()); i++) {
-              if (stack.length() > 0) {
-                stack.append("\n")
-                     .append(framePadding)
-                     .append("  at ");
-              }
+      if (stacksize < 1) {

Review comment:
       whoops, some auto-formatting seems to have kicked in.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org


[GitHub] [lucene] dweiss commented on a change in pull request #295: LUCENE-10099: Add -Ptests.asyncprofile option.

Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #295:
URL: https://github.com/apache/lucene/pull/295#discussion_r707042543



##########
File path: gradle/testing/profiling.gradle
##########
@@ -43,6 +44,28 @@ allprojects {
         }
       }
     }
+
+
+    if (resolvedTestOption("tests.asyncprofile").toBoolean()) {
+      allprojects {
+        tasks.withType(Test) {
+          jvmArgs("-javaagent:" + project.rootProject.file('buildSrc/build/libs/buildSrc.jar').getAbsolutePath(), "-XX:+UnlockDiagnosticVMOptions",

Review comment:
       Here is another reason to move it to a composite - you could reference the project's artifacts as a regular dependency, not a static path.

##########
File path: help/tests.txt
##########
@@ -157,6 +157,19 @@ to increase the top-N count:
 
 gradlew -p lucene/core test -Ptests.profile=true -Ptests.profile.count=100
 
+nocommit - asyncprofile

Review comment:
       Maybe move to help/profile.txt as it seems to be a larger issue on its own rights?

##########
File path: gradle/testing/randomization/policies/tests.policy
##########
@@ -116,6 +116,14 @@ grant {
   permission java.lang.RuntimePermission "defineClass";
 };
 
+// Permissions for asyncprofiler
+grant {
+  // needed by jacoco to instrument classes

Review comment:
       copy-paste (jacoco)?

##########
File path: buildSrc/build.gradle
##########
@@ -38,3 +38,11 @@ dependencies {
   implementation "commons-codec:commons-codec:${scriptDepVersions['commons-codec']}"
 }
 
+jar {

Review comment:
       Why is this in buildSrc? I think it should be another module entirely (at the top-level) and just made a composite, much like dev-tools/missing-doclet currently is?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org