Initialize a crate for memory related tools #89

cheme · 2018-12-10T18:34:45Z

This PR is a first step for heapsize removal/replacement openethereum/parity-ethereum#9953 . It uses a fork of malloc_size_of (unpublished crate from servo project).

This PR create a new crate parity-util-mem:

allows defining a global allocator (target should be jemalloc), by using a feature. (not that there is no priority so if multiple allocator feature are defined the compilation will panic in a global allocator redefinition error).
plugs selected allocator to their heapsize measurement (selection of allocator with a feature).
allows measurement alternative with only size_of or size_of_val : this can be activated quickly by setting estimate-heapsize feature.
move code from parity-ethereum mem crate (MemZero), and and clear_on_drop reexport. This is from a previous split commit, I can put if out of this pr if needed.

After this crate cration, the parity-ethereum base code will basically look like: openethereum/parity-ethereum@master...cheme:rem_heapsize . I may overuse the derive macro but I think it makes it more consistent (you do not need to check heapsize implementation when adding a field), there is also still some room for some implementations (especially if we want to use ConditionalMallocSizeOf, I need to test if there is some need).

Parity-common kvdb crate (otherwhise it will segfault for other allocator than malloc, because it still uses old heapsize directly).

The malloc_size_of usage is a bit awkward:
We copy code from servo project and patch it to remove some contents. We also switch servoarc to std arc.
I put a .sh file as documentation of the process and a .patch file to allow update from servo repo (I do not think there will be a lot).

The reason for this choice are :

using as an external crate does not allow defining trait for custom types (parity-types) in the crate. That is very problematic when it comes to update things (even with local projects the dependency on parity-types are really difficult to change. Therefore including the trait in the parity crate let us implement the trait for those types and have an easier switching (we can consider moving the implementation in fixed-hash and uint at some point but it is difficult as a first step (if we plan to I shall feature gate those implementations to make the switch possible without republishing the crate).
using as an external crate by specifying github address is also awkward due to the size of the servo github project (for malloc_sizeof_derive crate it is doable right now).
the crate itself is really small.

(it is the less impacting approach).

`get_malloc_size_src.sh`. Add missing impls.rs with ethereum types related implementation.

Fix a no_std deps issue.

dvdplm · 2018-12-11T08:15:26Z

Cargo.toml

@@ -17,5 +17,6 @@ members = [
 	"trace-time",
 	"trie-standardmap",
 	"triehash",
-	"uint"
+	"uint",
+  "mem",


mem is too generic. Can you change to measure-mem or malloc-size-of or some other more descriptive name?

will use the package name 'parity-util-mem'

dvdplm · 2018-12-11T08:16:17Z

mem/Cargo.toml

@@ -0,0 +1,41 @@
+[package]
+name = "parity-util-mem"


I like it when the filename is the same as the package name.

dvdplm · 2018-12-11T08:18:17Z

mem/Cargo.toml

+jemalloc-global = ["jemallocator"]
+# implement additional types
+ethereum-impls = ["ethereum-types", "elastic-array", "parking_lot"]
+# Default malloc to conditional mettering


I don't understand this comment (and there's a typo: s/mettering/metering/).

dvdplm · 2018-12-11T08:18:26Z

mem/Cargo.toml

+# implement additional types
+ethereum-impls = ["ethereum-types", "elastic-array", "parking_lot"]
+# Default malloc to conditional mettering
+conditional-mettering = []


Suggested change

conditional-mettering = []

conditional-metering = []

dvdplm · 2018-12-11T08:18:48Z

mem/Cargo.toml

+ethereum-impls = ["ethereum-types", "elastic-array", "parking_lot"]
+# Default malloc to conditional mettering
+conditional-mettering = []
+# Use serde impl from mallocsizeof


Is it malloc-size-of?

dvdplm · 2018-12-11T08:44:03Z

mem/src/lib.rs

+/// This is a copy of patched crate `malloc_size_of` as a module.
+/// We need to have it as an inner module to be able to define our own traits implementation,
+/// if at some point the trait become standard enough we could use the right way of doing it
+/// by implementing it in our type traits crates. At this time a move on this trait if implemented 


Suggested change

/// by implementing it in our type traits crates. At this time a move on this trait if implemented

/// by implementing it in our type traits crates. At this time moving this trait

dvdplm · 2018-12-11T08:44:32Z

mem/src/lib.rs

+/// We need to have it as an inner module to be able to define our own traits implementation,
+/// if at some point the trait become standard enough we could use the right way of doing it
+/// by implementing it in our type traits crates. At this time a move on this trait if implemented 
+/// at primitive types level would impact to much of the dependency to be easilly manageable.


Suggested change

/// at primitive types level would impact to much of the dependency to be easilly manageable.

/// to the primitive types level would impact too much of the dependencies to be easily manageable.

I think this information should be available in the readme.

dvdplm · 2018-12-11T08:45:21Z

mem/src/lib.rs

+#[cfg(feature = "ethereum-impls")]
+pub mod impls;
+
+/// reexport clear_on_drop crate


Doc comments start with capital letter. Add a link to the crate.

dvdplm · 2018-12-11T08:50:16Z

mem/src/malloc_size.rs

+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or


We're mixing licenses here, MIT/Apache with GPL. Is that ok? Can you double check please?

It seems like MIT is compatible and apache v2 is only compatible with GPLv3 (not gpl v2 but we are v3). So it looks fine.

dvdplm · 2018-12-11T09:16:52Z

mem/src/malloc_size.rs

+//!   `<Box<_> as MallocSizeOf>::size_of(field, ops)`.
+
+
+// This file is patched at commit 5bdea7dc1c80790a852a3fb03edfb2b8fbd403dc DO NOT EDIT.


As I mentioned above, I think we should considering either forking or write our own crate inspired by this one. It seems like 90% of the code here is irrelevant to our needs and that many types we might want to measure need specialized code anyway.

forking: doable but we need to get src/impl.rs and src/sizeof.rs in the fork, I prefer using a single crate (in the fork scenario I would consider reexported through parity-util-mem). The thing that I do not like with the fork is that we fork servo repo (thinking twice after trimming all the other stuff it is the same as a smaller repo, just with a huge history : I do not know if cargo is doing shallow clone).

write our own crate: I am not comfortable with writing existing code, but this PR is in someway doing that.

Documentation enhancements. Explicits imports.

of any use).

tomaka · 2019-01-02T13:56:01Z

The reason for this choice are :

Why not copy-paste Servo's crate (with the proper licensing of course), instead of cloning it and patching it? It would be the first crate we fork.

cheme · 2019-01-02T14:02:40Z

The crate is actually copied, I just wanted to keep it synching remote change somehow easy (but it may really not be necessary). So I guess it is possible (just need to remove the script and a comment in the copied code).

Proper way to do it should be to fork the crate, but it seems a bit too much when you consider the servo crate size.

tomaka · 2019-01-02T16:58:16Z

Shouldn't we remove everything specific to Servo, though?

I was most in favour of not using a trait and having direct methods on everything, but the approach of this PR may be better. I'm in favour of it except for the fact that the crate is very large.

parity-util-mem/malloc_size_of_derive/LICENSE-APACHE

parity-util-mem/README.md

Co-Authored-By: cheme <[email protected]>

cheme · 2019-01-02T19:01:35Z

@tomaka I did change the patch to remove all servo related code (before it was deactivated by a feature), it makes the copied code smaller. Maybe some of the servo trait will prove useless (I did not plan to use the conditional one directly (need some testing to see if relevant)) and could also be removed.

…tools2

estimate.

ordian

Can we test estimate-heapsize on CI?

parity-util-mem/src/sizeof.rs

parity-util-mem/src/malloc_size.rs

parity-util-mem/Cargo.toml

stale

cheme · 2019-01-11T09:59:59Z

I just realize that publishing this to crates.io (requires for dependency such as 'memorydb' (the only one that really need an update), means that I need to publish 'malloc_size_of_derive' (by design .
I see 3 scenarii here:

ask servo team to publish 'malloc_size_of_derive' : probably not suitable as 'malloc_size_of' is unpublished.
rename the copy of crate in the pr to 'parity-util-mem-derive' and publish it as a parity crate, I really don't like that because there is really no change to the code.
rewrite the proc macro (not super interesting) and publish it as a parity crate.

tomaka · 2019-01-11T11:04:48Z

Since we will have our local version of malloc_size_of, it makes sense to me to have our local version of malloc_size_of_derive as well.
If Servo's trait diverges from us, the proc macro will likely diverge as well.

ordian · 2019-01-14T07:03:32Z

The windows CI failure is unrelated (I wonder whether we shouldn't have switched from appveyor).

@cheme Could you open a PR to switch parity-common to this crate before publishing it? I guess that would be a breaking change, so versions need to be bumped.

cheme · 2019-01-14T08:23:00Z

@ordian , I have two ways to do it : the breaking one (I got a branch ready: cheme/parity-common@mem-tools2...cheme:rem_heapsize ), or just a deprecating one ( cheme/parity-common@mem-tools2...cheme:deprecate_heapsize for parity eth we need to update memory db but I can in a first time just deprecate 'mem_used' method and create a new one that uses this crate.

I can open both PR before publishing it (with reference to github branch), so we can check ci. I can also open the parity ethereum PR before publishing (the changes on common are not really likely to fail).

cheme added 7 commits December 6, 2018 19:41

Mem tools by including trait and specific implementation in the crate

25270be

(it is the less impacting approach).

Plug malloc_size_of as a patched copy. Documentation is done by

94bb293

`get_malloc_size_src.sh`. Add missing impls.rs with ethereum types related implementation.

A few additional impl for parity-eth use

197a27d

Allocators.

2a0cc08

heapsize estimation for allocator without use_size fn.

418e03b

Init no_std compat.

6105606

Remove conditianal test code.

59b4d55

Fix a no_std deps issue.

cheme added the A0-pleasereview label Dec 10, 2018

dvdplm previously requested changes Dec 11, 2018

View reviewed changes

cheme added 3 commits December 11, 2018 17:04

Renaming of mem repo to crate name.

efadc31

Documentation enhancements. Explicits imports.

Remove conditional mettering for now (will need some testing to check if

b2f1708

of any use).

tab

fb127a7

cheme requested a review from tomaka December 12, 2018 15:17

pepyakin reviewed Jan 2, 2019

View reviewed changes

parity-util-mem/malloc_size_of_derive/LICENSE-APACHE Outdated Show resolved Hide resolved

parity-util-mem/README.md Outdated Show resolved Hide resolved

cheme and others added 2 commits January 2, 2019 19:41

Remove features get form malloc_size and simply delete code instead.

d5aa899

Update parity-util-mem/README.md

fd9dc92

Co-Authored-By: cheme <[email protected]>

cheme added 4 commits January 3, 2019 09:08

Missing windows import

3df427b

Merge branch 'mem-tools2' of github.com:cheme/parity-common into mem-…

da88f97

…tools2

Use self for winapi.

ef9a5b6

Correct import.

1b0b2e3

ordian mentioned this pull request Jan 3, 2019

Getting rid of heapsize step 1: miner openethereum/parity-ethereum#9977

Closed

cheme added 2 commits January 3, 2019 19:10

shallow size on Arc with malloc return -1 (max val), replacing with

5340912

estimate.

use cfg_if to makes feature choice more explicit.

8ada814

ordian reviewed Jan 9, 2019

View reviewed changes

parity-util-mem/src/sizeof.rs Outdated Show resolved Hide resolved

parity-util-mem/src/sizeof.rs Outdated Show resolved Hide resolved

parity-util-mem/src/sizeof.rs Outdated Show resolved Hide resolved

cheme added 2 commits January 9, 2019 17:40

Remove servo specific impls (unused).

61c60b0

estimate_heapsize in travis.

767dc90

cheme added 2 commits January 9, 2019 18:04

Merge branch 'master' into mem-tools2

9876b65

Merge branch 'master' into mem-tools2

a8a9b0e

tomaka approved these changes Jan 10, 2019

View reviewed changes

parity-util-mem/src/malloc_size.rs Outdated Show resolved Hide resolved

Remove last serde and remove servo_arc reference.

a31851e

ordian approved these changes Jan 10, 2019

View reviewed changes

parity-util-mem/Cargo.toml Outdated Show resolved Hide resolved

Add missing version

73e8c7e

ordian added A8-looksgood and removed A0-pleasereview labels Jan 10, 2019

cheme added the A1-onice label Jan 11, 2019

Use malloc_size_of_derive from crates.

64135ac

cheme removed the A1-onice label Jan 14, 2019

This was referenced Jan 14, 2019

Fix malloc_size_of for mac #93

Merged

Remove heapsize #94

Merged

cheme merged commit e388dcf into paritytech:master Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize a crate for memory related tools #89

Initialize a crate for memory related tools #89

cheme commented Dec 10, 2018 •

edited

Loading

dvdplm Dec 11, 2018

cheme Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

dvdplm Dec 11, 2018

cheme Dec 11, 2018

dvdplm Dec 11, 2018

cheme Dec 11, 2018

tomaka commented Jan 2, 2019

cheme commented Jan 2, 2019 •

edited

Loading

tomaka commented Jan 2, 2019

cheme commented Jan 2, 2019

ordian left a comment

cheme commented Jan 11, 2019

tomaka commented Jan 11, 2019

ordian commented Jan 14, 2019

cheme commented Jan 14, 2019 •

edited

Loading

	/// by implementing it in our type traits crates. At this time a move on this trait if implemented
	/// by implementing it in our type traits crates. At this time moving this trait

	/// at primitive types level would impact to much of the dependency to be easilly manageable.
	/// to the primitive types level would impact too much of the dependencies to be easily manageable.

		//! `<Box<_> as MallocSizeOf>::size_of(field, ops)`.


		// This file is patched at commit 5bdea7dc1c80790a852a3fb03edfb2b8fbd403dc DO NOT EDIT.

Initialize a crate for memory related tools #89

Initialize a crate for memory related tools #89

Conversation

cheme commented Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka commented Jan 2, 2019

cheme commented Jan 2, 2019 • edited Loading

tomaka commented Jan 2, 2019

cheme commented Jan 2, 2019

ordian left a comment

Choose a reason for hiding this comment

cheme commented Jan 11, 2019

tomaka commented Jan 11, 2019

ordian commented Jan 14, 2019

cheme commented Jan 14, 2019 • edited Loading

cheme commented Dec 10, 2018 •

edited

Loading

cheme commented Jan 2, 2019 •

edited

Loading

cheme commented Jan 14, 2019 •

edited

Loading