1799806 - Profiler misreports Ion frames as Baseline frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Mayank Bansal]

Go to https://codepen.io/Mamboleoo/pen/NWYQMwR

Profile: https://share.firefox.dev/3UHtv9G
I was looking at the "copy" function that profiler suggests spends 85% time in Baseline

Comment 1

[Jan de Mooij [:jandem]]

Looking at this locally in the profiler, that copy function does seem to be a bit weird even though it's a tiny and trivial function.

I'll take a closer look later this week to see if it's there's anything weird going on.

Comment 2

[Jan de Mooij [:jandem]]

I looked at this more. What's happening is that the profiler gets confused when the sampler pc is in Ion IC code. At the end of JSJitProfilingFrameIterator::JSJitProfilingFrameIterator we then assume it's Baseline code.

We should probably add an IonEntry to the profiler's jitcode table for Ion IC code.

I can reproduce with something like this:

function g(o) {
    o.x = 1;
    o.y = 2;
}
function f() {
    with (this) {};
    for (var i = 0; i < 100_000_000; i++) {
        var o = {};
        var p = {a: 1};
        g(o);
        g(p);
    }
}
f();

Comment 3

[Jan de Mooij [:jandem]]

I also noticed something unexpected with the profiler UI: if I go to the stack chart, invert the stack, and hover over a JS function stack frame, sometimes the popup displays "Category: JavaScript" instead of the sub category (for example "Category: JavaScript: JIT (Baseline)").

I don't understand yet what's causing that.

Comment 4

[Jan de Mooij [:jandem]]

Let's keep this in the JS component because the issue in comment 2 will require a JS engine fix.

Comment 6

[Mayank Bansal]

Profile after bug 1801875: https://share.firefox.dev/3VosuUh
I am guessing there is more work required here.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Mayank Bansal from comment #6)

Profile after bug 1801875: https://share.firefox.dev/3VosuUh
I am guessing there is more work required here.

Yeah that bug is just a refactoring, it shouldn't have any observable differences.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1799806 - Add entries for Ion IC code to the profiler's code table. r?iain!

This fixes an issue where the profiler reported samples in Ion IC code as Baseline code.

This patch is based on bug 1614622 part 1, where we removed similar code for the old
Ion IC system, but a bit simplified after changes in bug 1801875 and other bugs.

There might be similar issues with trampoline code. Eventually it would be nice to
report time in Baseline IC code, Ion IC code, and Trampoline code separately in the
profiler, but that needs some refactoring of how frames are reported.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce553434720c
Add entries for Ion IC code to the profiler's code table. r=iain

Comment 10

[Mayank Bansal]

Could there be cases where the profiler misreports that we are spending time in Ion, but we are not?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Mayank Bansal from comment #10)

Could there be cases where the profiler misreports that we are spending time in Ion, but we are not?

Not that I'm aware of, but the profiler has some code that's hard to reason about.

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/ce553434720c

Comment 13

[Mayank Bansal]

Profile with latest Nightly: https://share.firefox.dev/3OJNN0o