[PATCH] D44847: [profile] Fix value profile runtime merging issues

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/profile/InstrProfilingFile.c:230
+  // (which can leave garbage at the end of profile file).
+  if (!VPMergeHook)
+    truncate(ProfileFileName, 0);
----------------
davidxl wrote:
> xur wrote:
> > davidxl wrote:
> > > xur wrote:
> > > > davidxl wrote:
> > > > > xur wrote:
> > > > > > davidxl wrote:
> > > > > > > non value profiling data has fixed size, so why is this needed?
> > > > > > Because there are still value counts in both in-memory and in-disk profiles.
> > > > > > 
> > > > > > Using the test in the patch as the example (running on my machine).
> > > > > > assuming the binary is named as 'gen' and VPMergeHook null.
> > > > > > 
> > > > > > if we don't do online merge,
> > > > > > "gen" will generate a profile of size 320 bytes.
> > > > > > "gen 1" will generate a profile with size of 352 bytes
> > > > > > 
> > > > > > If we run "gen 1 && gen", the result binary is still 352 bytes.
> > > > > > But the effective size is 320 bytes. The tail 32 bytes are trash.
> > > > > > 
> > > > > > 
> > > > > Without VpMergeHook, we should assert that there is no value profile data -- it should not be dumped in the first place.
> > > > current behavior is to write out the in-memory value profile data and discard the value profile data in the file. if there is not value profile from the file, the result is still correct.
> > > > 
> > > > I'm not familiar with the use cast the not having libc (where VPMergeHook will be nullptr). Is it not suppose to have any value profile?
> > > > 
> > > > I can assert VPMergeHook==nullptr and in-memory valuesites are not empty. That will be easy. Is this what you suggested?
> > > In lprofGetVPDataReader method, returns null if VPMergeHook is null.
> > This will generate incorrect profile: the valuesite in the data section says there are valuesites but VP count is empty.
> > 
> > if we do this, the behavior would be: llvm-profdata show or merge command will stop silently at the first function that has the value-site instrumentation. The rest profile will be discarded. 
> > 
> > Note, of course, this only applies to  VPMergeHook==nullptr case.
> On the other hand, it seems safe to do truncation here unconditionally before the writing later.
one correction. When Merge is done, the namedata section will be skipped during dumping to speed up the profile write. Make sure you do the truncation to the right size.


https://reviews.llvm.org/D44847