Proposal: add Discriminator field to Line message about pprof HOT 6 CLOSED

I think we should add one or the other, having both column and discriminator fields would look confusing IMO - like two identifiers of a position or fragment within the source line.

Maybe we should add the discriminator field with the comment that for languages that use DWARF to resolve the debug info this is the DWARF discriminator and other languages like JS can give a special meaning to it like column number.

from pprof.

having both line and discriminator fields would look confusing

I assume you mean "column and discriminator fields" here?

Maybe we should add the discriminator field with the comment that for languages that use DWARF to resolve the debug info this is the DWARF discriminator and other languages like JS can give a special meaning to it like column number.

This seems reasonable to me. w.r.t. DWARF, I'll note that the spec quote above says: "Discriminator values ... distinguish among multiple blocks that may all be associated with the same source file, line, and column. Where only one block exists for a given source position, the discriminator value is be zero (sic)."

My read of that is that the discriminator should only be set if the column is insufficient to discriminate, which would mean we'd need both. However, LLVM does not appear to follow the spec. In addDiscriminators, only filename + line is considered (L208). Column number is ignored. This makes sense considering that LLVM profile format only contains line + discriminator.

from pprof.

I assume you mean "column and discriminator fields" here?

Sorry, yes. Updated the text retroactively.

My read of that is that the discriminator should only be set if the column is insufficient to discriminate, which would mean we'd need both. However, LLVM does not appear to follow the spec.

Thanks for pointing out this detail. Yes, our internal symbolization pipeline also only carries the line number and the discriminator, not also the column number, so practically I think the two should be sufficient.

from pprof.

There is #818 in review currently which will address #687 (adding column field in the Line message). I don't think we will be adding both fields unless a strong use case emerges, so when the discriminator is needed I would propose using the column field for it. Closing this issue as "not planned" - please feel free to re-open if the proposed solution does not work.

from pprof.

when the discriminator is needed I would propose using the column field for it.

The downside to this is that today Go's PGO uses standard CPU profiles emitted from runtime/pprof, etc. If we put a discriminator rather than a column number in the "column" field, it will be confusing to anyone analyzing CPU profiles, as we seem to be reporting incorrect nonsense column numbers.

from pprof.

I think it may be a sign that a more specialized format would be a better thing here then. Including every possible bit of information in profile.proto can make hard to manage over time.

from pprof.