Troubles with parity-util-mem

[ordian]

parity-util-mem currently has several issues:

1. Having multiple version of parity-util-mem with different global allocator features set will lead to Undefined Behaviour, see #Ban multiple versions of parity-util-mem polkadot#922 (this is addressed in Ban duplicates of parity-uil-mem from being linked into the same program #363).
2. Setting a global allocator outside of parity-util-mem features may lead to UB (e.g. https://github.com/paritytech/polkadot/pull/918/files#diff-0d754f08395fefc53a33b67bd7e02a22R31).

I don't know how to address p. 2.

cc @cheme

[cheme]

For 2, the issue is that parity-util-mem need to be aware of the allocator in use, so that is why it should be this crate that define the allocator.
More generally any crate can define the allocator and break because another crate defines it too (so it is not strictly related to parity util mem).
I think this is nothing new, I was just trying to point out that I do not really see a different way of running thing.
Starting from those assumption, I see two case:

• Manage globally: the second crate defining the allocator need to link over parity-util-mem, as usual with cargo there may be some issue due to both different feature being imported, but in the linked code it can be all related to the use of no_std (and/or std feature dependant).
  But the problem will still be here, due to possibly two incompatible feature being called (cfg_if could be use to give priority: no_std for instance should priorize dlmalloc).
• Try to avoid first allocator definition: using parity-util-mem with feature 'estimate-size' do not define the allocator (this is not a good solution though).
  The second crate also could try to be clear of parity-util-mem, but that may be to difficult to achieve and lead to duplicate definition of data structure.

So clearly the definition of the allocator by feature in parity-util-mem should be done only at the top level (binary) to avoid having to manage this kind of thing, but for test it can be quite an annoyance.
We could also force some configuration for test, but I fear it will only complexify things.

Thinking about 1, the part of parity-util-mem that define allocator could also be extracted in another crate to make it more 'stable' and reduce the need for crate update (and have only a smaller crate that requires a single version, but having two different version of util-mem is probably rather difficult to do (you need very distinct data structure)).

TLDR; my first thought are other crate need to use parity-util-mem for their allocator definition, and splitting parity-util-mem crate can be a good idea (alloc definition in a sub crate).

[ordian]

More generally any crate can define the allocator and break because another crate defines it too (so it is not strictly related to parity util mem).

My understanding that if two crates define global allocators, it won't compile.

error: the #[global_allocator] in a conflicts with global allocator in: b

So the problem only happens if parity-util-mem doesn't define a global allocator, but it's defined somewhere in the code (e.g. see link above in p.2).

[cheme]

So the problem only happens if parity-util-mem doesn't define a global allocator, but it's defined somewhere in the code (e.g. see link above in p.2).

Parity util mem need to know which allocator is in use (except if runing with estimate-heapsize), so I would think using parity-util-mem in the link with feature dlmalloc is required, then there is two way to proceed:

• include parity-util-mem as a deps with dlmalloc feature for wasm tests and remove the global alloc definition.
• include parity-util-mem as a deps with dlmalloc feature, and modifiy parity-util-mem to not define the global alloc. Since we still need to add the deps I find first solution better.
  or third possibility
• include parity-util-mem as a deps with estimate-heapsize feature for wasm tests

Also if we want to get an error in such case, maybe we also want parity-util-mem to always define allocator (std::system::Allocator), but I am really not sure how things will behave.

[ordian]

Parity util mem need to know which allocator is in use (except if runing with estimate-heapsize), so I would think using parity-util-mem in the link with feature dlmalloc is required

I think for this dlmalloc specific case it's fine because it's used in a no-std environment where malloc_usable_size is not called, but it could easily be a UB otherwise.

Also if we want to get an error in such case, maybe we also want parity-util-mem to always define allocator (std::system::Allocator), but I am really not sure how things will behave.

This actually could work, good idea, let me try it.

[ordian]

Also if we want to get an error in such case, maybe we also want parity-util-mem to always define allocator (std::system::Allocator), but I am really not sure how things will behave.

Actually it doesn't seem to work as in it doesn't generate a link-time error.