Make wee-alloc an optional feature.

[yuval-k]

Allow building for non-wasm arch by only depending on wee allocator if we are compiling to wasm32.
Compiling to native has the advantages of being able easily write and run unit tests in dependent projects.

I did test this manually to work, but I am by no means a bazel expert.

[yuval-k]

@PiotrSikora given that I didn't really touched any code, how do you want to handle the failed outdated check?
I can update the dependency, but I'm not sure if that's desired.

[PiotrSikora]

@PiotrSikora given that I didn't really touched any code, how do you want to handle the failed outdated check?
I can update the dependency, but I'm not sure if that's desired.

Please ignore it. Those dependencies that were outdated before. I'll fix it shortly.

[gbrail]

FWIW I think that we should merge this or something like it.

I actually wish that the library didn't bring in wee-alloc at all. I don't think that it's the business of a library to set the global allocator on any platform, and WASM targets work just fine without it. The only downside is a few tens of K of generated code, which in any module I've written for Envoy is dwarfed by all the other stuff. Projects that want to shrink the size of their WASM can always include it themselves.

[PiotrSikora]

@gbrail the original reason for enabling this was indeed shrining the .wasm size, but a lot has changed since, and I agree that wee_alloc shouldn't be forced on users.

@yuval-k could you turn it into a crate features, by any chance?

[yuval-k]

i will give it a try; but not an expert on bazel machinery. will update soon

[yuval-k]

@PiotrSikora I added the feature to the rust side, but I'm not sure how to do it on the bazel side.. do you happen to know that off the top of your head? if not i'll have to dig in to the bazel rust rules...

i.e. i need to set the dependencies to rust_library conditionally, which i'm not sure how to do

[PiotrSikora]

You need to move this to src/allocator.rs and guard only #[global_allocator]. Having this guard here removes malloc() export.

[PiotrSikora]

Could you move [features] before [dependencies]?

[PiotrSikora]

Hmm... It looks that one of my responses got lost.

You need to regenerate BUILD files to match updated Cargo.toml:

cargo install cargo-raze --version 0.3.8
rm -rf bazel/cargo/
cargo generate-lockfile
cargo raze --output=bazel/cargo
mv Cargo.lock bazel/cargo/

Regarding Bazel, I couldn't find a way to make cargo-raze support features in the primary crate, but I think that we can skip wee_alloc there for now.

[PiotrSikora]

Please ignore it. Those dependencies that were outdated before. I'll fix it shortly.

Once #45 is merged, you should merge/rebase to fix outdated check. Sorry for the delay!

[yuval-k]

sounds good; will also add the raze comment to the readme

[yuval-k]

just tested this;
The problem with allow malloc is that it creates infinite recursion when not building for wasm:

#151 0x00005555555a6626 in malloc ()
#152 0x00005555555a5b0c in alloc::alloc::alloc ()
#153 0x00005555555a5c71 in <alloc::alloc::Global as core::alloc::AllocRef>::alloc ()
#154 0x00005555555a4743 in alloc::raw_vec::RawVec<T,A>::allocate_in ()
#155 0x00005555555a4a68 in alloc::raw_vec::RawVec<T,A>::with_capacity_in ()
#156 0x00005555555a445a in alloc::raw_vec::RawVec<T>::with_capacity ()
#157 0x00005555555a510a in alloc::vec::Vec<T>::with_capacity ()
#158 0x00005555555a6626 in malloc ()
#159 0x00005555555a5b0c in alloc::alloc::alloc ()
#160 0x00005555555a5c71 in <alloc::alloc::Global as core::alloc::AllocRef>::alloc ()
#161 0x00005555555a4743 in alloc::raw_vec::RawVec<T,A>::allocate_in ()
#162 0x00005555555a4a68 in alloc::raw_vec::RawVec<T,A>::with_capacity_in ()
#163 0x00005555555a445a in alloc::raw_vec::RawVec<T>::with_capacity ()
#164 0x00005555555a510a in alloc::vec::Vec<T>::with_capacity ()
#165 0x00005555555a6626 in malloc ()
#166 0x00005555555a5b0c in alloc::alloc::alloc ()
#167 0x00005555555a5c71 in <alloc::alloc::Global as core::alloc::AllocRef>::alloc ()
#168 0x00005555555a4743 in alloc::raw_vec::RawVec<T,A>::allocate_in ()
#169 0x00005555555a4a68 in alloc::raw_vec::RawVec<T,A>::with_capacity_in ()
#170 0x00005555555a445a in alloc::raw_vec::RawVec<T>::with_capacity ()
#171 0x00005555555a510a in alloc::vec::Vec<T>::with_capacity ()
#172 0x00005555555a6626 in malloc ()
...

To fix this I added #[cfg(target_arch = "wasm32")] to mod allocator

[PiotrSikora]

@yuval-k ah, so your issue is not with wee_alloc itself after all, but malloc? Or both? Could you try #42?

Note that even if #42 fixes your issue, I'd still like to get merge this PR for adding wee_alloc as optional feature.

[yuval-k]

it's both; wee_alloc (when doesn't target wasm) brings in libc create; so it's easier to not include it than include both

[PiotrSikora]

@yuval-k could you merge/rebase on top of master with merged #42 and try to only guard #[global_allocator]? Removing malloc / proxy_on_memory_allocate doesn't seem correct, since it's part of the ABI.

[yuval-k]

yes, that works!

[PiotrSikora]

One more thing, could you please add Build (wasm32-wasi with wee-alloc) and Clippy (wasm32-wasi with wee-alloc) with --features=wee-alloc to both stable and nightly jobs in .github/workflows/rust.yml?

[PiotrSikora]

Nit: s/You/you/

[PiotrSikora]

Nit: )) -> )

[PiotrSikora]

Nit: )) -> )

[PiotrSikora]

Thanks!