| commit | ac544388df245f6fddaa57a8266907d8f6602499 | |
| author | Shaunak Kishore <kshaunak@fb.com> | |
| Wed, 5 Jan 2022 00:08:42 +0000 (4 16:08 -0800) | ||
| committer | Facebook GitHub Bot <facebook-github-bot@users.noreply.github.com> | |
| Wed, 5 Jan 2022 00:10:04 +0000 (4 16:10 -0800) | ||
| tree | f62ef85f5fba5f755b8fd77d469979c72cde9eaa | treesnapshot (tar.gz zip) |
| parent | 34a37a646e17525060ac9bbf8e4c69852f82f59b | commitdiff |
Alternate fix for RDS profiling bug
Summary:
This diff is an alternate version of D32809514 (https://github.com/facebook/hhvm/commit/3b92b772cddca8a6064decbfa7ae8b37ef22347a) that still allows profiling on non-request threads. The original version of this diff is a noticeable performance hit. I'm not completely sure why that is, but I have two guesses:
1. That diff disables profiling for RDS locals. ricklavoie and I are pretty sure that that is the case. The problem is that we allocate the profile for an RDS local before starting a request, so it appears as if they're allocated on non-VM threads.
1. The cost of doing this check is actually high in some place that matters - for example, where we collect our binaries FDO/BOLT data.
This diff should address both cases, by simplifying the "is profiling" check to a single atomic load and by profiling on non-VM threads again. Instead of limiting which threads we profile, we solve the original crash bug by stopping profiling when we hit retranslateAll. We'll always hit RTA before we start the serialization thread.
Note that we previously used "shouldProfileAccesses" to mean two different things (that happened to coincide before this diff):
1. To check if we should *start* profiling RDS in a given runtime configuration.
1. To check if we are *currently* profiling RDS accesses.
I've changed it so that "shouldProfileAccesses" ALWAYS means the former, and introduced a new "rds::profiling" helper that means the latter. "shouldProfileAccesses" is rarely used. It appears in some assertions, at runtime option parsing, and in one other place: when we unbind a link, we attempt to unbind its profiling handle based on this flag, so that we'll clean up profiling handles even after retranslateAll.
Reviewed By: mofarrell
Differential Revision: D33354852
fbshipit-source-id: d7fef8cd9906a8fb070e273c688c7cdae46675fe
Summary:
This diff is an alternate version of D32809514 (https://github.com/facebook/hhvm/commit/3b92b772cddca8a6064decbfa7ae8b37ef22347a) that still allows profiling on non-request threads. The original version of this diff is a noticeable performance hit. I'm not completely sure why that is, but I have two guesses:
1. That diff disables profiling for RDS locals. ricklavoie and I are pretty sure that that is the case. The problem is that we allocate the profile for an RDS local before starting a request, so it appears as if they're allocated on non-VM threads.
1. The cost of doing this check is actually high in some place that matters - for example, where we collect our binaries FDO/BOLT data.
This diff should address both cases, by simplifying the "is profiling" check to a single atomic load and by profiling on non-VM threads again. Instead of limiting which threads we profile, we solve the original crash bug by stopping profiling when we hit retranslateAll. We'll always hit RTA before we start the serialization thread.
Note that we previously used "shouldProfileAccesses" to mean two different things (that happened to coincide before this diff):
1. To check if we should *start* profiling RDS in a given runtime configuration.
1. To check if we are *currently* profiling RDS accesses.
I've changed it so that "shouldProfileAccesses" ALWAYS means the former, and introduced a new "rds::profiling" helper that means the latter. "shouldProfileAccesses" is rarely used. It appears in some assertions, at runtime option parsing, and in one other place: when we unbind a link, we attempt to unbind its profiling handle based on this flag, so that we'll clean up profiling handles even after retranslateAll.
Reviewed By: mofarrell
Differential Revision: D33354852
fbshipit-source-id: d7fef8cd9906a8fb070e273c688c7cdae46675fe