-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Direct allocation attribution in profiling #1777
Conversation
Rebase, and rewrite. The main change in the rewrite is that I'm not creating the new thread local field any more, thanks to #1779. |
tsd_thread_allocated_last_event_get(tsd); | ||
size_t sample_wait = tsd_prof_sample_event_wait_get(tsd); | ||
if (accumbytes < sample_wait) { | ||
/* Don't bother to set surplus - it will never be read. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may trigger some warning. Let's see...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected. Adding an initializer.
Fix compiler warnings, as well as one other place revealed by the FreeBSD unit tests: |
Rebase on top of #1787 to get rid of the warning. |
@@ -101,11 +101,11 @@ arena_prof_tctx_reset_sampled(tsd_t *tsd, const void *ptr) { | |||
} | |||
|
|||
JEMALLOC_ALWAYS_INLINE void | |||
arena_prof_info_set(edata_t *edata, prof_tctx_t *tctx) { | |||
arena_prof_info_set(edata_t *edata, prof_tctx_t *tctx, uint64_t surplus) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surplus should be a size_t, no?
* purpose, because a valid surplus value is strictly less than | ||
* usize. | ||
*/ | ||
*surplus = usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I might suggest: I'm not sure I would spot that *surplus == usize
is invalid while debugging. Some other garbage constant (e.g. 0x99999999
or 0xdeadbeef
or something) OTOH, I would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Use SIZE_MAX
instead. (Strictly speaking 0x99999999
and 0xdeadbeef
can be valid surplus
values.)
Revise according to feedback. |
Rebase, and added another field My next step is to define an opt-in run-time option and, when it is turned on, to ingest the surplus values in the dump content. |
Rebase. |
Rebase, and added:
|
Closing. #1897 serves to correct the bias. Will reopen if there's any need for inference, in addition to a single point estimate. |
This change is not completed yet, but I think it has reached a stage suitable for seeking some early feedback.
The context is in #1751. To enable correct inference, we need to record both the number of sampled bytes and the number of unsampled bytes for each stack trace. The number of sampled bytes is equivalent to
curobjs
, so we only need to record the number of unsampled bytes, which I denote assurplus
- the amount beyond what's needed for triggering sampling.surplus
is relayed in two routes:thread_alloc_event()
->prof_malloc_sample_object()
, via a new thread local fieldprof_sample_event_surplus
, which is needed because the sampling logic is currently separate from the thread event logic;prof_malloc_sample_object()
->prof_free_sampled_object()
, via a new fielde_prof_surplus
onedata
, which is needed becausefree()
needs to roll back the allocation'ssurplus
from the stack trace total.My remaining work involves the following:
surplus
. (I figured that computing thesurplus
is fairly cheap so I don't really need to guard the computation under any option.)surplus
value as well as some other convenient estimation(s) to the profiling dump.