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.

Sign up for GitHub

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

Jump to bottom

Provide #[derive(MallocSizeOf)] that is actually working #291

Merged
merged 12 commits into from
Dec 31, 2019
Merged

Provide #[derive(MallocSizeOf)] that is actually working #291

merged 12 commits into from
Dec 31, 2019

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Dec 21, 2019

On top of #290

@ordian ordian requested a review from cheme December 21, 2019 19:05
@NikVolf NikVolf mentioned this pull request Dec 23, 2019
Merged
dvdplm
dvdplm suggested changes Dec 30, 2019
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Changing malloc_size.rs means we essentially "fork" from the Servo version (we're at commit 5bdea7dc1c80790a852a3fb03edfb2b8fbd403dc).
That's not necessarily a problem but something we should be aware of (and clean up that code comment and maybe the readme?).

parity-util-mem/tests/derive.rs Outdated Show resolved Hide resolved
parity-util-mem/tests/derive.rs Show resolved Hide resolved
parity-util-mem/derive/Cargo.toml Outdated Show resolved Hide resolved
parity-util-mem/Cargo.toml Show resolved Hide resolved
@NikVolf
Copy link
Contributor Author

NikVolf commented Dec 30, 2019

@dvdplm addressed

dvdplm
dvdplm reviewed Dec 30, 2019
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

A few minor nits and then there's the matter of testing #[ignore_malloc_size_of = …].

parity-util-mem/src/malloc_size.rs Outdated Show resolved Hide resolved
parity-util-mem/src/malloc_size.rs Outdated Show resolved Hide resolved
dvdplm
dvdplm approved these changes Dec 31, 2019
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm

@NikVolf
Copy link
Contributor Author

NikVolf commented Dec 31, 2019

@ordian @niklasad1 Hey guys can you also take a look?

ordian
ordian reviewed Dec 31, 2019
View reviewed changes
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

AFAIR #[derive(MallocSizeOf)] actually worked if we renamed parity-util-mem to memory_size_of (example), but I agree it is a hack.

parity-util-mem/derive/lib.rs Show resolved Hide resolved
parity-util-mem/tests/derive.rs Show resolved Hide resolved
@NikVolf
Copy link
Contributor Author

NikVolf commented Dec 31, 2019

AFAIR #[derive(MallocSizeOf)] actually worked if we renamed parity-util-mem to memory_size_of (example), but I agree it is a hack.

Yeah, hacky, might bring some issues later

@NikVolf NikVolf merged commit 4612d11 into master Dec 31, 2019
@NikVolf NikVolf deleted the nv-mem-utils-2 branch December 31, 2019 16:03
@cheme
Copy link
Collaborator

cheme commented Jan 1, 2020

Lgtm too, about the way to include external module (parity-util-mem) into the derive macro, I like the current substrate way of doing thing (since we got the derive macro in our code base we can probably use it now): https://github.com/paritytech/substrate/blob/20a9b15cdbed4bf962a4447e8bfb812f766f2fbc/frame/support/procedural/tools/src/lib.rs#L34 (I do not remember to well how it works but it is actually well tested on substrate).
With the code forked, it may be also possible to add a serde like implementation for a specific field with a function (I remember some discussion about it and I am not sure it is worth the work).

@ordian
Copy link
Member

ordian commented Jan 1, 2020

With the code forked, it may be also possible to add a serde like implementation for a specific field with a function (I remember some discussion about it and I am not sure it is worth the work).

You mean #255? Yeah, it's possible to implement it now.

dvdplm added a commit that referenced this pull request Jan 14, 2020
@dvdplm
Merge branch 'master' into dp/chore/secp256k1-from-upstream
48a2ef6 
* master: (27 commits)
  update parity-util-mem (#309)
  Update features and feature dependencies (#307)
  Use proper memory queries to rocksdb (#308)
  Draft version updates and changelog (#299)
  Use custom error type for `from_hex` (#305)
  Fix typo. (#303)
  kvdb: remove KeyValueDBHandler (#304)
  extract common kvdb tests into a crate (#301)
  Expose to_hex and from_hex from impl-serde (#302)
  Add a top level function to parity-util-mem (#298)
  I/O statistic for key-value databases (#294)
  use for_tuples (#300)
  Add memory extensions for LRUCache, hashbrown (#293)
  Add memory stats for kvdb-s (#292)
  Provide #[derive(MallocSizeOf)] that is actually working (#291)
  Move and extend impls for locking primitives (#290)
  Update uint README (#288)
  Update README.md (#287)
  [ci]: remove feature flags in virtual workspace (#289)
  Prepare releases (#286)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@ordian ordian ordian approved these changes

@cheme cheme Awaiting requested review from cheme

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@NikVolf @cheme @ordian @dvdplm