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

[Wenlei He via Phabricator via llvm-commits]

wenlei marked an inline comment as done.
wenlei 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>;
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66191