Skip annotating invalid frames

[eljamm]

This is useful for stack traces that have more information than the file:line count format.

For example, the Nix language's evaluator profiler also includes information about the function being called, which sometimes has a non-canonical call site location and therefore cannot be annotated.

Here is how this might look like:

<nix/derivation-internal.nix>:37:12:primop derivationStrict

As such, with this change, we effectively skip annotating frames that don't conform to the file:line count format.

Code portion from the NGIpkgs overview.

[t-troebst]

Its not uncommon for profilers to generate some stack frames that don't really correspond nicely to source locations (e.g. because a call went through an external library that we don't have any source information for). For this reason, when we cannot parse a file and line number, the code is supposed to treat that particular frame as essentially a black box.

This is done through a bit of a hack where instead storing a file and line number, we set file = "symbol" (just a fake value that shouldn't clash with a real absolute file path) and line number = whatever the stack frame is. For this reason, file should never be nil. Its useful to still have these symbols that don't correspond to code locations in the actual callgraph; you wouldn't want to omit external library calls from your profiling information just because they are not useful for annotation (since they are useful for the find hottest functions).

So if file is nil, that means frame_unpack is violating its contract (since it explicitly says that it cannot return nil). Looking at the code, I think the line parsing variant of the function has some issues compared to the table-based one that is used for perf, i.e. this code:

local symbol, file, linenr = frame:match("^(.-)%s*(/.+):(%d+)$")

if symbol and file and linenr then
    if symbol == "" then
        symbol = nil
    end

    return symbol, vim.loop.fs_realpath(file), tonumber(linenr)
end

Firstly, we don't cache the file lookups here which is not great since it can be slow. But more importantly, this can probably return nil unlike its contract says if vim.loop.fs_realpath fails. Thats why the code above has a fallback here. Also, in the pattern I guess %s* should probably be %s+ - we expect at least one whitespace character between the symbol and the file.

[eljamm]

you wouldn't want to omit external library calls from your profiling information just because they are not useful for annotation (since they are useful for the find hottest functions).

Forgive my ignorance, but in these functions, those black boxes don't appear to have contents in them. Is this just the case for the current profiler or am I missing something? For example, in PerfHottestLines:

So if file is nil, that means frame_unpack is violating its contract (since it explicitly says that it cannot return nil).

Ah, I see. That does make sense, and indeed I can annotate again if I just add a check for vim.loop.fs_realpath.

Also, in the pattern I guess %s* should probably be %s+ - we expect at least one whitespace character between the symbol and the file.

This might be related to the Nix flamegraph format, but this actually breaks annotation for me as no symbols are matched.

By the way, here is a real example of what the flamegraph looks like (from the post I linked above): nix.profile.gz, if that helps in any way.