Add --nostop to always start profiler recording even when --ttsp or --start/--end are used #1046

openorclose · 2024-11-05T14:49:18Z

Description

Added --nostop to always start profiler recording even when --ttsp or --start/--end are used

Related issues

#833

Motivation and context

In particular, this allows us to continue recording events in --ttsp mode

How has this been tested?

Added a test for the original ttsp mode and also the newly added flag.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

apangin · 2024-11-11T23:52:45Z

src/arguments.cpp

                _mcache = value == NULL ? 1 : (unsigned char)strtol(value, NULL, 0);

+            CASE("recordonly")
+                _start_alongside_traps = true;


It's hard to search and follow the logic, when asprof argument, agent argument and the variable in the code are all named differently. Let's use the same name if possible.
My suggestion is nostop (similarly to nostop option for handling signals in gdb), but other suggestions are welcome.
Consequently, arguments and the variable name will be nostop, --nostop, _nostop.

apangin · 2024-11-12T00:04:52Z

src/main/main.cpp

+    "  --timebetween a b record when a is executed, and then when b is executed, and duration between\n"
+    "  --onlyttsp        only time-to-safepoint profiling \n"
+    "  --includettsp     profile as per normal, but include time-to-safepoint information\n"


I understand you wanted to shortcut certain use cases, but having many options to do the same thing causes unnecessary ambiguity or even confusion, considering that asprof arguments do not match agent arguments.
If we add just one new option (e.g. --nostop as mentioned above), this will be unambiguous, will match agent argument, and will cover all use cases relatively simply. Moreover, it will remain backward compatible.
IMO, --ttsp --nostop is not much worse than --includettsp.

I would probably agree with additional --window A B option to record profiler.Window event between execution of A and B, but this is a quite rare use cases, so it does not deserve a separate option, I think.

Will use nostop for now, anyone else feel free to suggest others

apangin · 2024-11-12T00:05:52Z

src/main/main.cpp

            params << ",begin=SafepointSynchronize::begin,end=RuntimeService::record_safepoint_synchronized";

+        } else if (arg == "--includettsp") {
+            fprintf(stderr, "ZZZZ");


Debug leftover.

apangin · 2024-11-12T00:08:46Z

test/test/jfr/JfrTests.java

+    public void includettsp(TestProcess p) throws Exception {
+        p.profile("-d 5 --includettsp -f %f.jfr");
+        if (!containsSamplesOutsideWindow(p)) {
+            throw new RuntimeException("Expected to find samples outside of ttsp window, but did not find any");


Same as above.

apangin · 2024-11-12T01:10:15Z

test/test/jfr/JfrTests.java

+        }
+    }
+
+    private boolean containsSamplesOutsideWindow(TestProcess p) throws Exception {


Can be shortened and simplified a bit.

events list is redundant; we may collect profiler windows and execution samples in the first pass.

Profiler windows can be stored in a TreeMap as startTime->endTime. This will provide sorting and lookup for the closest window out of the box.

Search loop can be simplified with stream().anyMatch().

apangin · 2024-11-12T01:18:43Z

test/test/jfr/Ttsp.java

+import java.io.BufferedOutputStream;
+import java.io.FileOutputStream;
+
+class Ttsp {


This example needs some comments. How does it expose long TTSP issue? Why getAllStackTraces() is called in a loop? Writing 1GB files in a loop may not be a good thing to do in a test - IO is not what we are testing here. Furthermore, not all systems may even have a spare gigabyte in /tmp volume.

Thank you, I adapted this example from an online article regarding debugging ttsp issues, will rewrite with explanation

apangin · 2024-11-12T01:20:55Z

Thank you for the PR.
The feature is indeed useful, but I think it can be implemented in a simpler way - added some comments inline.

This reverts commit 6838f04.

Signed-off-by: Andrei Pangin <1749416+apangin@users.noreply.github.com>

apangin · 2024-12-09T01:16:36Z

test/test/jfr/Ttsp.java

+    static private byte loop() {
+        byte[] byteArray = new byte[1024 * 1024 * 1024];
+        for (int i = 0; i < 10000; i++) {
+            new Random().nextBytes(byteArray);


Random.nextBytes contains a loop with a safepoint check - it does not cause long time-to-safepoint pauses.

apangin · 2024-12-09T01:17:28Z

test/test/jfr/Ttsp.java

+    public static void main(String[] args) throws Exception {
+        new Thread(() -> {
+            while (true) {
+                System.gc();


ThreadMXBean.dumpAllThreads is a much cheaper way to force global safepoint.

apangin · 2024-12-09T01:18:29Z

test/test/jfr/Ttsp.java

+            }
+        }).start();
+
+        Thread.sleep(1000);


Relying on Thread.sleep in tests in error-prone.

apangin · 2024-12-09T01:23:46Z

test/test/jfr/Ttsp.java

+        new Thread(() -> {
+            while (true) {
+                loop();
+            }


A loop in this example is not very useful. The only long ttsp pause happens the very first time when allocating new byte[1024 * 1024 * 1024] array; there are no ttsp pauses afterwards.

apangin · 2024-12-09T01:25:59Z

Changes look good, except for the test, which does not really demonstrate time-to-safepoint pauses.
I'll merge this PR and push a revised test afterwards.

Allow recording windows between a start and stop function

365f219

openorclose closed this Nov 5, 2024

openorclose deleted the ttsp branch November 5, 2024 15:45

openorclose restored the ttsp branch November 5, 2024 15:46

openorclose reopened this Nov 5, 2024

Add copyright headers

9ba22a3

apangin reviewed Nov 12, 2024

View reviewed changes

Daryl Tan added 9 commits November 25, 2024 15:18

Merge remote-tracking branch 'origin/master' into ttsp

0ad85dd

Address PR review comments

7d21a6b

Merge remote-tracking branch 'origin/master' into ttsp

6dde474

Update TTSP tests

89bbf1d

Update TTSP tests

fbac090

Merge remote-tracking branch 'origin/master' into ttsp

74fb7ff

Update ProfilerOptions.md

6838f04

Revert "Update ProfilerOptions.md"

5501cc4

This reverts commit 6838f04.

Update ProfilerOptions.md

11992c2

openorclose changed the title ~~Allow recording windows between a start and stop function~~ Add --nostop to always start profiler recording even when --ttsp or --start/--end are used Dec 6, 2024

apangin and others added 2 commits December 9, 2024 00:20

Updated documentation and formatting

d9f9087

Merge branch 'master' into ttsp

c63a385

Signed-off-by: Andrei Pangin <1749416+apangin@users.noreply.github.com>

apangin reviewed Dec 9, 2024

View reviewed changes

apangin merged commit d7ab642 into async-profiler:master Dec 9, 2024
10 checks passed

Add --nostop to always start profiler recording even when --ttsp or --start/--end are used #1046

Add --nostop to always start profiler recording even when --ttsp or --start/--end are used #1046

Uh oh!

Conversation

openorclose commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Motivation and context

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apangin commented Nov 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apangin commented Dec 9, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

openorclose commented Nov 5, 2024 •

edited

Loading