[PATCH] D66191: [AutoFDO] Make call targets order deterministic for sample profil

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:146
 public:
-  using CallTargetMap = StringMap<uint64_t>;
+  using CallTargetMap = std::map<StringRef,uint64_t>;
 
----------------
wenlei wrote:
> wmi wrote:
> > StringMap is more effient than std::map, but I assume the call targets list won't be very long so using std::map may not be an issue for compile time. 
> > 
> > A problem of map is that if we want to sort CallTargetMap, I like to see the targets are sorted in count order (high to low) instead of alphabet order, so the dump will be more meaningful and we don't need sort the targets again using SortCallTargets in SampleProfile.cpp. But map cannot be internally sorted in its value_type.
> > 
> > How about leave CallTargetMap as a StringMap and make SortCallTargets a member function of SampleRecord and sort CallTargetMap before dumping and file writing? 
> Thanks for review. Yeah, I think the number of targets is so small that efficiency benefit of StringMap is not important. 
> 
> I would actually prefer the targets to be in alphabetical order - that makes the textual profile more 'stable' thus easy to compare (line by line) for count difference. 
> 
> Besides, having to sort before dump moves the sorting responsibility to SampleProfileWriter for all difference formats, that's kind of duplicated, and doesn't prevent adding future writer without sorting before dumping. 
> 
> SortCallTargets currently converts StringMap<uint64_t> to SmallVector<InstrProfValueData> first, then sort in place, if we extract a common Sort helper, we would need an extra copy to hold sorted result before converting to InstrProfValueData. The overhead won't be visible, but just feel it's not that clean either.
> 
> I don't have strong opinion, but do prefer to leave it as. Let me know if that is ok. Thanks.
> I would actually prefer the targets to be in alphabetical order - that makes the textual profile more 'stable' thus easy to compare (line by line) for count difference.

Even you order the targets in alphabetical order, there may be some targets showing up in one profile and disappearing in another profile -- causing a lot of churn for profile comparison. Usually the top call targets are what you concern the most. If the top call targets are scattered in a long list and their changes are amid some other targets's changes, it is easy to miss some of them.  




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66191