User Details
- User Since
- Apr 3 2019, 5:55 PM (234 w, 2 d)
Mon, Sep 18
lgtm assuming comments for constants will be added. thanks.
Mon, Sep 11
lgtm, thanks!
Tue, Sep 5
lgtm, thanks.
Because as you point out later sample profile loaders etc. actually trying hard to avoid the ,0 case by adding +1 which feels like people maybe do not want it to be legal and then we could have re-used it :)
Meaning !{"branch_weights", i32 1, i32 0} being an abstract representation of a "likely" branch.
Mon, Sep 4
lgtm, thanks.
This is a good change. Thanks.
Fri, Sep 1
Thu, Aug 31
lgtm, thanks.
Thanks for attempting the clean up. I think the original version is probably more readable. I left comments there. wdyt?
This brings up to 8% speed up (31.4s vs 29.0s) when reading a large test profile, and 5% speedup (0.82s vs 0.78s) when reading the function offset table alone.
Aug 30 2023
- 0.02% less instructions from the unordered_map version.
- 1.2% less wall clock time from the unordered_map version.
We didn't measure the time spent within sample profile loader though, the above numbers are all e2e compilations.
Aug 28 2023
lgtm with nits. thanks.
Aug 25 2023
lgtm, thanks.
The stack with refactoring first makes the changes easier to review. Thanks!
lgtm, thanks.
lgtm except nits, thanks.
We have something similar internally, but didn't upstream because we are not sure if the use case is too narrow to justify burdening the code base with all the related complexity. cc @hoy
lgtm, thanks.
looks cleaner and more structured now, thanks for update!
That said, even if we still want to attempt the change to use DenseMap, I suggest we try that in a separate patch, so the majority of the original changes can get in and be stabilized first, and others can be unblocked now without needing to revert the whole patch. Evidently attempts to remove reference stability is non-trivial and also needs more testing, which warrants a change of its own.
Aug 24 2023
For the current sample loader, if the root or parent function profile are lost or mismatched, all its profile and its children profile are dropped, we should use the non-flattened profile(the original nested profile) to count the total mismatched samples.
Aug 23 2023
I agree that hard coded probability is not great. But I also don't think there is a one-size-fit-all answer here. EH path can be unlikely, z-trip loop can be unlikely, [[unlikely]] hint can be unlikely, but they don't necessarily share the same probability.
Fix a dangling reference bug in ProfileConverter::flattenNestedProfile . This is an existing bug only revealed after D147740 changes the profile map container type.
Aug 22 2023
Aug 16 2023
thanks for the fix and following up with the comments, lgtm now.
Aug 15 2023
IIRC, mcf in spec has a function sort_basket which is known to be problematic for maintaining loop rotate counts, if you want to play with it a bit, especially the guessing part of the heuristic.
I think the heuristics are better than doing nothing in theory. Though one other thing to note is that for AutoFDO, if the profiled build also has the loop rotated, the incoming counts won't be right to begin with -- if the two branches after rotate are executed A times and B times, the original branch should be executed A+B times, but AutoFDO will annotate it with max(A,B) times for the original branch.
Aug 14 2023
Aug 8 2023
Jul 17 2023
Jul 14 2023
lgtm, thanks.
Jul 13 2023
In general if the effort to make sure probe has zero discriminator is too much, it makes sense to just rely on discriminator clean up instead during FS discriminator assignment for probes. I'm curious though what exposed this case? IIRC, we didn't hit this assert for a while now.
Jul 10 2023
lgtm, thanks.
lgtm, thanks.
Jul 7 2023
nit: update the change description too based on the new comment (describing it using push/pop isn't very intuitive). otherwise lgtm.
Jul 6 2023
Jul 5 2023
Jun 28 2023
lgtm, thanks!
Jun 27 2023
lgtm, thanks.
lgtm, thanks.
Can this be achieved by simply tweaking ProfiledCandidateComparer so zero SizeCost candidate always comes before non-zero ones?
Jun 26 2023
lgtm, thanks.
Jun 25 2023
Copying comments from the original patch for continuation..
Thanks for the refactoring and cleanup. LGTM.