[PATCH] D155290: [PGO] Use Unique Profile Files when New Processes are Forked

Wed Aug 23 07:40:45 PDT 2023

qiongsiwu1 added a comment.

Thanks for chiming in @phosek !

In D155290#4587438 <https://reviews.llvm.org/D155290#4587438>, @phosek wrote:

> Can we avoid the use of `pthread_atfork` and the dependency on pthreads? Using pthreads inside the profile runtime implementation is problematic in the case where you want to instrument the libc itself which is used on some platforms and it's one of the reasons why we would like to eliminate the dependency on libc altogether and use `sanitizer_common` instead, see https://discourse.llvm.org/t/using-c-and-sanitizer-common-in-profile-runtime/58629.

Yes that is something I will try. The case illustrated in `https://discourse.llvm.org/t/using-c-and-sanitizer-common-in-profile-runtime/58629` makes sense. Is there something similar to `pthread_atfork` in `sanitizer_common` you would recommend? This functionality needs a hook at fork, and it does not really rely on pthreads.

> In D155290#4585607 <https://reviews.llvm.org/D155290#4585607>, @qiongsiwu1 wrote:
>
>> In D155290#4582825 <https://reviews.llvm.org/D155290#4582825>, @MaskRay wrote:
>>
>>> Using `%p` should not magically change the behavior and I am unsure the result is better for PGO. 
>>> If we decide to provide such a mode, it needs to be opt-in by adding a new format specifier or API.
>>> Can you elaborate separate profile files are going to be useful?
>>
>> Thanks again for taking a look at this patch!
>>
>> Sure! We are generating separate profiles through `%p` for two reasons. First, we think that `%p` implies a profile per process, even if `exec` is not called after `fork`. It is unexpected that with `%p`, a program that forks child processes only create one profile file from the parent. This is why we did not create a new API or create a new pattern. Second, we are trying to catch non-gracefully terminated processes during profile generate. We observed a scenario where a hot function having all 0 counters. The reasons was the child process was created by a fork (but no `exec` followed), and then was terminated probably by a signal, instead of going through `exit()`. Such a process did not have a chance to write back the profile for merging. The parent process took a different call path and never called the hot function. We would like to detect such a situation for a particular process. Hence this patch creates an empty profile file if a process is not terminated gracefully. Using `%p` is probably the most natural choice when debugging such issues.
>>
>> One can argue that we may look into the continuous mode to manage abnormal terminations during profile generate. I have not looked too closely at that, but we think it is still useful for a user to tell that some processes did not write any profile back, for debugging, or for diagnostics.
>
> Handling cases like these is exactly why the continous mode was created. Your solution only covers a single case, but there's many scenarios where process can terminate abnormally and fail to invoke `atexit` hooks which would results in profile not being written out.

Yes I agree there is overlap with continuous mode. That said, there is still the first reason that we need to implement this feature, i.e. no new profile file is created at fork when `%p` is present in the file name pattern. My understanding is that `%p` implies a profile file per process created, see the new test case `compiler-rt/test/profile/Posix/instrprof-fork.c`. My expectation when running the test case is that we should have two different profile files, while currently we only have one profile file created. As @MaskRay pointed out in his comment for this test case, the profile files generated for the two processes can be very different. In the context of fork without exec, the current implementation will only generate one profile file. Effectively the result is identical with the profile generated without `%p`, if I understand the current implementation correctly. Additionally, continuous mode is not available on all platforms (for example AIX which is our use case). We are working on getting the continuous mode online, but `%p` as a debugging feature in the context of fork without exec seems to be useful.

With this said, if it is indeed our //intention// that new profile files should not be created upon fork when `%p` is present in the file pattern, I am happy to choose alternative routes. I would love to know why we have such an intention before moving on. I understand that this is an //existing// feature so we may be hesitant to change its behavior, and we would rather not have dependency on `pthread`, which is an implementation issue. Are there additional reasons that we prefer the current behavior?

Thanks again for the input!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155290