[PATCH] D100235: [CSSPGO][llvm-profgen] Fixing an obselete iterator issue.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D100235#2681312 <https://reviews.llvm.org/D100235#2681312>, @wenlei wrote:

>> Another issue being fixed is that all outlined functions shared the same inline frame previously due to the unchanged Index value as the dummy inlineSite identifier.
>
> Why is this an issue? What problem does this cause? Conceptually, if we want to give them a tree structure, top level inliner are indeed children of dummy root.

Yes, top level inliner is still children of dummy root with this change. However, they do not share dummy callsite id any more. Note that without this change, the dummy callsite id for top-level inliner (`Index` below) isn't updated correctly.

  if (Root != Cur) {
     Index = readUnsignedNumber<uint32_t>();
   }



>> They could be used to visit all probes that are associated to an inline frame, which is something else I'll be working on.
>
> We do have containers that can be re-allocated for growth, and doing that itself isn't an issue, unless it breaks specific use case. With what we have now we should not store pointers to inlinee's probe, and we're indeed doing that right now. So there's not bug, right?
>
> If there's a future change that require changing this model (of not keeping probe pointers), perhaps make the vector->list change together with the change that has the new use case?

It doesn't break anything so far since `PseudoProbeInlineTree.ProbeVector` is not used anywhere. It might also be good to remove it for now since the container might end up with invalid pointers.

Probe pointers isn't an issue as long as they are valid. I'm using `std::list` instead of `std::vector` to store real probe objects so that taking address of them is never invalidated.

I'm actually working on a feature that reports all dangling probes in an inline frame into a profile, even if there are no samples collected for them. As of now only dangling probes with samples are reported. Since samples collected on a dangling probe doesn't make sense (dangling probes can be placed anywhere), they should always be reported so that the counts inferencer never overlooks them. It makes sense to include the current patch into that work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100235/new/

https://reviews.llvm.org/D100235