Add cpu sampling #1286

JugadK · 2025-05-08T02:18:55Z

Description

Adds a "cpu" virtual frame at the bottom of each stack when one passes in --record-cpu as a flag

Related issues

Motivation and context

Discussed in the issue, but I want to be able to see which cpu a sample was taken on, this is done through enabling PERF_EVENT_CPU for the perf profile when --record-cpu is passed as a flag. This is read from the ring buffer into StackContext which is made into a virtual frame at the bottom of the stack (Similar to --sched).

How has this been tested?

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

JugadK · 2025-05-09T20:42:44Z

src/frameName.cpp

+        case BCI_CPU: {
+            int cpu = (int)(uintptr_t)frame.method_id;
+            char buf[32];
+            snprintf(buf, sizeof(buf), "%d", cpu-1);


an alternative idea I had was to make the line in the beginning of this function (Instead of having the +1 offset)

if (frame.method_id == NULL) {

be

if(frrame.bci != BCI_CPU && frame.method_id == NULL)

so it will skip this if method_id is zero, not familiar with the codebase though so not sure if that would have a bad side effect?

I'd set some bit instead - to make sure method_id is never NULL and to distinguish it from BCI_THREAD, which uses the same encoding.

Like just set the left most bit then unset it when we are reading it? that works

hm what if I shift left by one and set, then shift right when reading? don't have to make assumptions on the size of a pointer on the system

apangin · 2025-05-09T22:15:13Z

src/arguments.cpp

            CASE("inverted")
                _inverted = true;

+            CASE("record-cpu")


This is a section for FlameGraph options as the comment above states.
I'd put new option next to sched, which is kind of similar behavior-wise.

apangin · 2025-05-09T22:17:36Z

src/frameName.cpp

+        case BCI_CPU: {
+            int cpu = (int)(uintptr_t)frame.method_id;
+            char buf[32];
+            snprintf(buf, sizeof(buf), "%d", cpu-1);


I'd set some bit instead - to make sure method_id is never NULL and to distinguish it from BCI_THREAD, which uses the same encoding.

apangin · 2025-05-09T22:19:26Z

src/frameName.cpp

+            int cpu = (int)(uintptr_t)frame.method_id;
+            char buf[32];
+            snprintf(buf, sizeof(buf), "%d", cpu-1);
+            return _str.assign("[cpu").append(buf).append("]").c_str();


Why building string part-by-part, if you can format the whole thing in the above snprintf?

originally I was using std::to_string() and appending, will change it to snprintf

apangin · 2025-05-09T22:21:50Z

src/perfEvents_linux.cpp

 #ifdef PERF_ATTR_SIZE_VER5
    if (_cstack == CSTACK_LBR) {
-        attr.sample_type |= PERF_SAMPLE_BRANCH_STACK | PERF_SAMPLE_REGS_USER;
+        attr.sample_type |= PERF_SAMPLE_BRANCH_STACK | PERF_SAMPLE_REGS_USER | PERF_SAMPLE_CPU;


Have you checked this actually works? It seems that the field is always added, but the reader only parses it when _record_cpu is set.

ah this is from when i was hacking it together not sure why I left it in (it should only be enabled in the if statement block below

apangin · 2025-05-09T22:23:36Z

src/profiler.cpp

+      // +1 is to prevent the method_id from being read as a null pointer
+      // when cpu = 0
+      num_frames += makeFrame(frames + num_frames, BCI_CPU, java_ctx.cpu + 1);


Please fix indentation.

apangin · 2025-05-09T22:24:10Z

src/stackWalker.h

    const void* pc;
    uintptr_t sp;
    uintptr_t fp;
+    u64 cpu;


Why did you change it to 64-bit?

ring.next() returns u64 so i thought it was nicer to just keep it as the type that was originally returned. Though I don't think it makes much of a difference

ok, thanks for the explanation

…d being null when cpu is 0

apangin · 2025-05-10T15:49:09Z

src/stackWalker.h

+        this->cpu = 0;
+    }
+
+    void set(const void* pc, uintptr_t sp, uintptr_t fp, u64 cpu) {


Where is this function used?

It is not, since it seems like we have a function to set all fields in that class I thought it would make sense to add another function with the option of passing CPU for completion sake/future usage. Can remove if you feel its not needed

apangin · 2025-05-10T15:51:52Z

src/perfEvents_linux.cpp

+                  u64 cpu = ring.next();
+                  java_ctx->cpu = cpu;


Use 4 spaces indent.
IMO, local variable is redundant here, just set java_ctx->cpu = ring.next();

apangin · 2025-05-10T15:55:32Z

src/profiler.cpp

        num_frames += makeFrame(frames + num_frames, BCI_ERROR, OS::schedPolicy(0));
    }
+    if (_add_cpu_frame) {
+        num_frames += makeFrame(frames + num_frames, BCI_CPU, (java_ctx.cpu << 1) | 0x1);


I thought of setting a higher bit instead. In this case, there will be no need for shift, and frame values for BCI_CPU will not clash with values for BCI_THREAD.

I liked this approach as it prevents as errors from occurring from mistakes in casting, changes in intermediary datatypes and different pointer/int sizes on various platforms (i.e one of them being smaller than whatever nth bit we set) in practice just setting the 16th bit would probably have the same effect so I can do that

Im curious what you mean by "clashing". they are differentiated by the bci enum, but is there some problem with them being the same value in the method_id pointer? maybe I'm missing something.

The same value for method_id can be a problem as long as MethodMap uses jmethodID as a key.

apangin · 2025-05-10T15:56:57Z

src/profiler.cpp

        _add_event_frame = args._output != OUTPUT_JFR;
        _add_thread_frame = args._threads && args._output != OUTPUT_JFR;
        _add_sched_frame = args._sched;
+        _add_cpu_frame = args._record_cpu;


Since record_cpu is only supported with PerfEvents, do you want to return an error when record_cpu is requested for any other engine?

yeah that sounds like a good idea

apangin · 2025-05-12T23:08:39Z

src/stackWalker.h

        this->pc = pc;
        this->sp = sp;
        this->fp = fp;
+        this->cpu = 0;


This seems to overwrite cpu value set earlier. Did you test different stack walking modes (fp, dwarf, vm, default)?

hm yeah default,vm,vmx work but fp, dwarf do not, think i'll just remove setting it as 0

apangin · 2025-05-12T23:52:52Z

src/arguments.cpp

 //     filter=FILTER    - thread filter
 //     threads          - profile different threads separately
 //     sched            - group threads by scheduling policy
+//     record-cpu       - record which cpu a sample was taken on (default: false)


Do you mind updating ProfilingOptions.md doc as well?

Thanks. Can you also add a sanity test for --record-cpu? Ideally, all new features should be covered by at least one test. Alternatively, you may modify one of existing --target-cpu tests by adding --record-cpu to it.

… walkers

Signed-off-by: JugadK <68133163+JugadK@users.noreply.github.com>

apangin · 2025-05-13T01:40:08Z

test/test/cpu/CpuTests.java

+    public void perfEventsRecordCpuEventsCount(TestProcess p) throws Exception {
+
+        Output outRightCpu = p.profile("-d 2 -e cpu-clock -i 100ms --total -o collapsed --record-cpu");
+        assertCloseTo(outRightCpu.total(), 2_000_000_000, "perf_events total should match profiling duration");


This does not actually test the feature.
I suggest you pin the process to CPU 1 and check that output contains [CPU-1], but not [CPU-0].

ah thought you mean't by "sanity test" just something that sees if it does not crash, will do

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

apangin · 2025-05-13T14:30:53Z

All good now. Thank you for the contribution!

jkhajuria added 11 commits May 7, 2025 19:09

add --record-cpu flag

e38ac72

write cpu frame to jfr file is frame.bci is BCI_CPU

958adfa

name and type for BCI_CPU

b887a95

eanble perf cpu sampling if record_cpu is enabled

ead5ff2

add _record_cpu to perfEvents.h

cf6872b

add virtual cpu frame to profiler if record_cpu is true

20650d4

add bci_cpu frame type

76ee058

add a u32 cpu type for java_ctx

a6863bb

stop adding cpu twice

fea4679

resize buffer for printing frame name

7a8ff36

formatting

915324c

JugadK commented May 9, 2025

View reviewed changes

JugadK marked this pull request as ready for review May 9, 2025 20:46

jkhajuria added 2 commits May 9, 2025 14:39

standardize "cpu" variable integer type

b519a1f

formatting

07d93f1

apangin reviewed May 9, 2025

View reviewed changes

jkhajuria added 5 commits May 9, 2025 17:06

instead of a +1 offset use a shift + set lowest bit to avoid method_i…

bb9d74a

…d being null when cpu is 0

move record-cpu closer to sched

a4c3e5a

remove adding PERF_SAMPLE_CPU when CSTACK_LBR is true

8c8b677

move _record_cpu in arguments.h closer to sched

17f7901

fix grammar

dd6ffdf

apangin reviewed May 10, 2025

View reviewed changes

jkhajuria added 3 commits May 12, 2025 15:15

remove unneeded intermediary variable for ctx cpu field

307aa25

set 16th bit instead of shifting for cpu method_id encoding

2de890c

remove unused set() function in stackwalker

1fe5559

apangin reviewed May 12, 2025

View reviewed changes

jkhajuria and others added 3 commits May 12, 2025 17:09

remove setting cpu on StackContext set() as it breaks fp, dwarf stack…

e262e27

… walkers

Update ProfilerOptions.md to add --record-cpu flag

fd3c5e2

Signed-off-by: JugadK <68133163+JugadK@users.noreply.github.com>

add record-cpu sanity test

66214f1

move record cpu failures into own tests

cd69b37

apangin reviewed May 13, 2025

View reviewed changes

jkhajuria and others added 3 commits May 12, 2025 19:18

check output for correct cpu frames for record-cpu flag

4bfb7af

Edits

51c45f6

Edits

18240d5

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

apangin merged commit 7c3aa59 into async-profiler:master May 13, 2025
17 checks passed

apangin mentioned this pull request May 13, 2025

Fetch which CPU core a sample was taken on #1240

Closed

2 tasks

Add cpu sampling #1286

Add cpu sampling #1286

Uh oh!

Conversation

JugadK commented May 8, 2025 • 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!

JugadK May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JugadK May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JugadK May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JugadK May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JugadK commented May 8, 2025 •

edited

Loading

JugadK May 9, 2025 •

edited

Loading

JugadK May 9, 2025 •

edited

Loading

JugadK May 9, 2025 •

edited

Loading

apangin May 12, 2025 •

edited

Loading

JugadK May 13, 2025 •

edited

Loading