Skip to content
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

track-allocation: charge entry cost to caller #34391

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 15, 2020

This effectively makes two related changes:

  1. Completely removes one-time costs of runtime functions (aka "warmup costs") from being charged to the user's code. This was already being partly subtracted in some cases (depending on whether we hit a malloc record point) resulting in both over- and under- reporting of the user's allocations. However, theoretically these costs are implementation details that the user doesn't specifically care about, and should thus be charged against the internal allocation budget instead. (This implements should allocation tracking ignore compilation by default? #19981 and should make Profile.clear_malloc_data() less necessary, and just get the desired answer on the first pass). In the future, we can continue to sharpen this if people report cases I've missed. We can also even create a bin to put these in so that they continue to be reported, but just charged against the correct cause.
  2. Adjust where entry point costs are charged against. Previously we charged the callee for any setup costs incurred by the caller. That sometimes wasn't too bad, but other times would result in very large numbers being added the the first instruction in the user's function. Now, we'll charge those against the caller, so you won't see that happen anymore. Instead those costs should now show up on the line of code that actually incurred them.

@vtjnash vtjnash added backport 1.4 feature Indicates new feature / enhancement requests labels Jan 15, 2020
@KristofferC
Copy link
Member

Would it be better to let this go into 1.5. It doesn't seem that urgent?

Don't include one-time costs (JIT compilation) so that warm-up isn't generally required.
And adjust codegen emission to charge call entry costs to the caller.

fixes #11753
fixes #19981
fixes #21871
fixes #34054
close #18595
@vtjnash vtjnash force-pushed the jn/no-track-alloc-internal branch from d5f64c5 to 1adcc8f Compare January 21, 2020 20:38
@vtjnash vtjnash merged commit 91a118e into master Jan 27, 2020
@vtjnash vtjnash deleted the jn/no-track-alloc-internal branch January 27, 2020 19:09
vtjnash added a commit that referenced this pull request Jan 28, 2020
On 32-bit, this object falls into the 12-byte pool, which does not exist
on some platforms (such as Win32). Query for which pool this will land
into and test the track-allocation result accordingly.

Fixes a test added in #34391
vtjnash added a commit that referenced this pull request Jan 29, 2020
On 32-bit, this object falls into the 12-byte pool, which does not exist
on some platforms (such as Win32). Query for which pool this will land
into and test the track-allocation result accordingly.

Fixes a test added in #34391
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
Don't include one-time costs (JIT compilation) so that warm-up isn't generally required.
And adjust codegen emission to charge call entry costs to the caller.

fixes #11753
fixes #19981
fixes #21871
fixes #34054
close #18595
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
On 32-bit, this object falls into the 12-byte pool, which does not exist
on some platforms (such as Win32). Query for which pool this will land
into and test the track-allocation result accordingly.

Fixes a test added in #34391
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants