Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Getting rid of heapsize step 1: miner #9977

Closed
wants to merge 1 commit into from
Closed

Conversation

mocsy
Copy link

@mocsy mocsy commented Nov 27, 2018

size_of_val in std seems to work fine.
This would allow this project to get rid of heapsize crate dep.

May or may not help out with #9953

@parity-cla-bot
Copy link

It looks like @mocsy hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@mocsy
Copy link
Author

mocsy commented Nov 27, 2018

[clabot:check]

@parity-cla-bot
Copy link

It looks like @mocsy signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added this to the 2.3 milestone Nov 27, 2018
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 27, 2018
@mocsy
Copy link
Author

mocsy commented Dec 1, 2018

6 byte is the size of the data of:
let bytes: Vec<u8> = vec![1, 15, 31, 63, 127, 255];
On windows both heap_size and std::mem gives 6, that's fine.

Using heapsize the linux story is different:
On stable it claims data is padded to 2^n, which is 8 in this case.
On nightly reports 32bit which is wrong, since nightly no longer uses jemalloc I could actually measure using heaptrack and std::mem is spot on.

I propose to measure the data size using std::mem and remove heapsize.
I updated the test I provided accordingly.

mem_size

Note: heap_size_of_children needs to be removed later when all packages use heap_size_of_data instead.
In this PR I updated miner only.

@mocsy
Copy link
Author

mocsy commented Dec 11, 2018

In my attempt to understand all this, I hit on a comment which is possibly relevant:

servo/heapsize#80 (comment)

My understanding is the goal here is to generate statistics and for that size_of_val should work fine.
size_of_val tells us the size of the data, and not how many bytes the allocator would allocate.
A custom allocator could reserve more bytes than the data needs. This might not tell if the allocator uses extra spacce for something or not. The std feature is still useful to recognize if data has been freed or not, and the number would be more consistent between platforms.
The other option would be to use allocator specific functions.
Heapsize remains jemalloc specific, and malloc_size_of looks specific to me as well.

@mocsy
Copy link
Author

mocsy commented Dec 11, 2018

What block the usage of heapsize is rustc switching migrating off of jemalloc.
heapsize remains jeamalloc only as you found servo/heapsize#80

On #9953 two approaches were discussed, switching back to jemalloc and keep using heapsize, or embracing the new default and stop depending on heapsize.

This PR proposes a way to do the second.

About heapsize being precise: it assumes jemalloc and it's precise as long as jemalloc is used, but if you look at the screenshot and data I posted above, it shows it's not precise with the new version of rust.

@5chdn
Copy link
Contributor

5chdn commented Dec 28, 2018

needs a 2nd reviewer, @ordian @seunlanlege anyone? :)

@ordian
Copy link
Collaborator

ordian commented Jan 3, 2019

@mocsy thanks for your contribution. The fix for #9953 is being worked on in paritytech/parity-common/pull/89.

I believe mem::size_of_val doesn't fully fit our purposes, e.g. for a Vec it shows its size and not capacity (the actual amount of allocated memory). So I'm closing this PR.

@ordian ordian closed this Jan 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants