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

Account for the old generation in memory measurements #216

Merged
merged 1 commit into from
May 1, 2018

Conversation

PragTob
Copy link
Member

@PragTob PragTob commented May 1, 2018

The erlang gc information seems to report old and young generation
separately.

Here is an example from faulty measurements we used:

[
  old_heap_block_size: 0,
  heap_block_size: 610,
  mbuf_size: 77,
  recent_size: 0,
  stack_size: 14,
  old_heap_size: 0, # <--- notice me
  heap_size: 540,
  bin_vheap_size: 0,
  bin_vheap_block_size: 46422,
  bin_old_vheap_size: 0,
  bin_old_vheap_block_size: 46422
]

[
  old_heap_block_size: 2586,
  heap_block_size: 1598,
  mbuf_size: 0,
  recent_size: 198,
  stack_size: 14,
  old_heap_size: 355, # <--- notice me
  heap_size: 275,
  bin_vheap_size: 0,
  bin_vheap_block_size: 46422,
  bin_old_vheap_size: 0,
  bin_old_vheap_block_size: 46422
]

Notice how heap_size got smaller but together with the
old_heap_size memory was still consumed?

Yup, that's what we're talking about here.

So, always take both of them into account.

Solves #213 when combined with #214 - wanted to split it out because one is about handling faulty values in statistics/formatting and this one is nice and separate in the memory measurement department.

The erlang gc information seems to report old and young generation
separately.

Here is an example from faulty measurements we used:

```elixir
[
  old_heap_block_size: 0,
  heap_block_size: 610,
  mbuf_size: 77,
  recent_size: 0,
  stack_size: 14,
  old_heap_size: 0, # <--- notice me
  heap_size: 540,
  bin_vheap_size: 0,
  bin_vheap_block_size: 46422,
  bin_old_vheap_size: 0,
  bin_old_vheap_block_size: 46422
]

[
  old_heap_block_size: 2586,
  heap_block_size: 1598,
  mbuf_size: 0,
  recent_size: 198,
  stack_size: 14,
  old_heap_size: 355, # <--- notice me
  heap_size: 275,
  bin_vheap_size: 0,
  bin_vheap_block_size: 46422,
  bin_old_vheap_size: 0,
  bin_old_vheap_block_size: 46422
]
```

Notice how `heap_size` got smaller but together with the
`old_heap_size` memory was still consumed?

Yup, that's what we're talking about here.

So, always take both of them into account.
Copy link
Collaborator

@devonestes devonestes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a way we could reliably test this, is there? I think this is good to go 👍 (and we should probably release 0.13.1 soon, too, to fix this), but if you can think of a way to test this then that would be good to add.

@PragTob
Copy link
Member Author

PragTob commented May 1, 2018

yeah sadly couldn't think of a way to test this... The only "test" I can think of is to inline Michals benchmarking suite and run an integration test against it... wanted to do that either way but I need both PRs merged for that. Will try that once both are merged :)

Thanks for the review 👍

@PragTob PragTob merged commit 3781a8e into master May 1, 2018
@PragTob PragTob deleted the account-for-old-generation-memory-measurement branch May 1, 2018 18:49
@devonestes
Copy link
Collaborator

Even that test will be tricky because of variation in memory usage between environments. I was seeing pretty significant differences between my local machine and CI. I guess we could run that benchmark and at least ensure that none of the measurements come back negative?

@PragTob PragTob mentioned this pull request May 1, 2018
@PragTob
Copy link
Member Author

PragTob commented May 1, 2018

@devonestes Oh just saw this. So, because the measurements were always the same and the new output code puts out N/A and the warning if things go horribly wrong we can test for their absence. So that's good. For me, it also seemed to work when eliminating all the other scenarios - so maybe even a unit test with these in the memory module would work... intriguing. See #218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants