Lift burden of single parity-util-mem version per Repo

[mustermeiszer]

Are there any plans for lifting the burden that only a single version of parity-util-mem is allowed per project?

I am not in the picture if this is solvable, but this restriction currently basically means that projects must use a single substrate version across all their dependencies. Due to the wiring of node and wasm this currently means node-substrate-version = wasm-substrate-version.

And correct me if I am wrong, this would also mean that projects that actually want to use the enum Call from other projects (e.g. for example for creating a Xcm<Call>) must be in sync with respect to substrate version being used.

[bkchr]

CC @cheme

[ordian]

The reason we have this restriction is because parity-util-mem is also used to setup a global allocator and if two version are used with different allocators, that will lead to nasty bugs.

[mustermeiszer]

@ordian do you believe there is a reasonable way to solve this anyways? Maybe by syncing the parity-util-mem version being used by substrate (would this even resolve this?) ?

[cheme]

I remember we thought splitting the crate in two (allocator choice part and alloc-size trait part) could help.
But in the descibred use case, you would still need that Call use the trait from the same util-mem crate than in Xcm<Call>, so it will not really help.

[mustermeiszer]

Okay. Yeah, thanks for the fast feedback. Currently Call is () anyways. So probably let's worry about this once xcm actually supports using enum Call from other chains.

I close this for now if there is no solution on the horizon.

[mustermeiszer]

One further question:

• What do you use the allocator for? For the wasm-instance or in general overall everywhere? (Sorry, I am pretty nooby wrt. what is even possible there.)

[bkchr]

FWIW, I would like to see this crate being removed from Substrate. We don't really use it and there is no real benefit in it. I would more like to switch to something that is being used in the Rust ecosystem (not sure if there currently exists such a crate).

[cheme]

FWIW, I would like to see this crate being removed from Substrate. We don't really use it and there is no real benefit in it. I would more like to switch to something that is being used in the Rust ecosystem (not sure if there currently exists such a crate).

Before using it we did use one that was being used in the rust ecosystem, but it stops being maintained and we had to use this (we use code from the servo project, which was the more likely to be the next thing getting use by the ecosystem but I think it never did happen).
At some point we did use https://crates.io/crates/malloc_size_of_derive (which is the only part of this code that was published), but I think it was rewritten and we now use parity-utli-mem-derive.

I did not look recently if maybe other crates could be use instead, but I would be pleasantly surprised if there was, and of course would find it great to switch.
(same thing if removal is possible it would be a good thing)

[cheme]

What do you use the allocator for? For the wasm-instance or in general overall everywhere? (Sorry, I am pretty nooby wrt. what is even possible there.)

it is the global allocator used by rust so could be use in the wasm build, but in practice I think we only use it to switch to jemalloc in clients builds.

[mustermeiszer]

Dumb question. Why is this not possible to use jemalloc? https://blog.rust-lang.org/2018/08/02/Rust-1.28.html#global-allocators

[Edit: Typos]

[cheme]

Dumb question. Why is this not possible to use jemalloc? https://blog.rust-lang.org/2018/08/02/Rust-1.28.html#global-allocators

[Edit: Typos]

That's what the crate does. (the thing that is really awkward with the crate is the trait that needs to be implemented by everyone, the allocator choice is just a small utility with little added value).

[cheme]

Sorry, it's been a long time since I look at this code.
Actually we are calling the allocator method internally, so the crate needs to know which allocator is used.
That's why it is convenient to set the global allocator in the crate.
If there was a standard way to get this (allocated ptr size) from the rust allocator trait, there would be no need to care about the choice of allocator (but trait would still be needed to recursively go into heap allocated struct).

[bkchr]

FWIW, I would like to see this crate being removed from Substrate. We don't really use it and there is no real benefit in it. I would more like to switch to something that is being used in the Rust ecosystem (not sure if there currently exists such a crate).

Before using it we did use one that was being used in the rust ecosystem, but it stops being maintained and we had to use this (we use code from the servo project, which was the more likely to be the next thing getting use by the ecosystem but I think it never did happen). At some point we did use https://crates.io/crates/malloc_size_of_derive (which is the only part of this code that was published), but I think it was rewritten and we now use parity-utli-mem-derive.

I did not look recently if maybe other crates could be use instead, but I would be pleasantly surprised if there was, and of course would find it great to switch. (same thing if removal is possible it would be a good thing)

Yeah I know where it originates, but the problem is also that our version is also not really maintained anymore. There are some fixes from time to time, but only if they are really required.

[mustermeiszer]

Just out of interest. Could I set my own global allocator in a dependency and then my project would use two different allocators? Or how does the compiler handle this?

And would any dependency that defines a global allocator affect the allocator being used?

[cheme]

You cannot define two global alloc. It will result in a compiling error :(

[cheme]

That 's why defining the global allocator in a library is a bad idea.
It should be define in the top level crate.
For parity util mem it means that we use the 'jemalloc' feature only in the top Cargo.toml of polkadot..

[mustermeiszer]

But then I don't understand the reason for having an artificially single version of this crate introduced .

The reason we have this restriction is because parity-util-mem is also used to setup a global allocator and if two version are used with different allocators, that will lead to nasty bugs.

I jemalloc flag is only set in top crate then this would mean those bugs can not occur?

[ordian]

We already had a bug due to two version of parity-util-mem: paritytech/polkadot#922.

[cheme]

The issue when using two different version is that you are using two different traits for MallocSizeOf, so then when you compose structure things will not work (eg XCM will impement the trait of parity-util-mem v0.8 but Call will implement the trait of parity-util-mem V0.9, then XCM will not implement any of those).

[cheme]

We already had a bug due to two version of parity-util-mem: paritytech/polkadot#922.

Oh yes, that's even worse :(

(a bad fix could have been to set the same allocator feature for both version, but it would be hell to maintain).

[mustermeiszer]

We already had a bug due to two version of parity-util-mem: paritytech/polkadot#922.

But this bug then only results from the wrong estimated size being used by MallocSIzeOf, right? And this results from the fact that in the lib.rs the global allocator is set but in the allocator.rs the extern "C" malloc_usable_size is set depending on the feature set, which subsequentially can lead to different allocators being used for estimating the heap size of an object?

Couldn't we set the extern "C" malloc_usable_size in the same config than the global allocator? This would implicitly force all projects to have the same feature flags for parity-util-mem, preventing those bugs mentioned above and at the same time lifting the burden of only allowing a single version of this crate. Wdyt?

Sorry, if I am annoying as hell here, and you can shortcut this if this solution is insufficient, but I really want this restriction to be gone 😄

[cheme]

That is similar to idea behind splitting the crate that I mentioned above.
You then have one crate with the traits definition. This crate is usable without fearing #922.
You have another crate that contain allocator specific code (choice, and actual malloc function calls), which keep all the previous problem.

So problem like #922 will only show when the crates actually call the size evaluation (which requires importing both split crate), and it is a better situation.
So splitting is a good idea.

But in the end it does only help you producing incompatible implementation of the malloc trait. It is probably the reason why I did not proceed to do the split at the time the idea was previously bring.

[cheme]

In itself I don't think there is an issue with splitting the crate, it is mainly extracting traits to another crate (derive crate will then use it and the current crate could even reexport so it does not require any other changes).

[mustermeiszer]

@cheme is there some way to test, if the #922 is safely prevented when playing around with this here? I mean, when I change the restriction it goes through, but not if its safe.

[cheme]

I drafted the split here https://github.com/cheme/parity-common/tree/split-mem, if your code depends only on the new crate, there is absolutely no code touching the global allocator, so I don't really think testing is worth it.

This quick draft makes me realize there is a bit of thing that can fail (missing feature reexport, change of feature condition), and I am really not sure anymore if it is worth merging/maintaining.