Stream: git-wasmtime

Topic: wasmtime / issue #5804 cranelift-entity: more efficient E...

Wasmtime GitHub notifications bot (Feb 16 2023 at 18:59):

jameysharp opened issue #5804:

Feature

The EntitySet type declared in cranelift/entity/src/set.rs is a fairly standard implementation of a bit-set. However, it uses 8-bit chunks in the EntitySet::elems storage vector. It should use a size closer to the width of a machine word.

Benefit

Using a larger size should give a small performance improvement.

Implementation

I'd prefer if elems were a Vec<usize>. Then the number of bits in each element is:

const BITS: usize = core::mem::size_of::<usize>() * 8

All the places which use the constant 8 should instead refer to BITS, and 7 should be replaced with BITS - 1.

If someone wants to tackle this, it would also be great if you could add doc-tests for the methods on this type while you're there. If you do, you may be able to remove the existing tests as they should be redundant with reasonably comprehensive doc-tests.

Alternatives

We can always leave this alone; it's not terrible as-is.

Wasmtime GitHub notifications bot (Feb 16 2023 at 18:59):

jameysharp labeled issue #5804:

Feature

The EntitySet type declared in cranelift/entity/src/set.rs is a fairly standard implementation of a bit-set. However, it uses 8-bit chunks in the EntitySet::elems storage vector. It should use a size closer to the width of a machine word.

Benefit

Using a larger size should give a small performance improvement.

Implementation

I'd prefer if elems were a Vec<usize>. Then the number of bits in each element is:

const BITS: usize = core::mem::size_of::<usize>() * 8

All the places which use the constant 8 should instead refer to BITS, and 7 should be replaced with BITS - 1.

If someone wants to tackle this, it would also be great if you could add doc-tests for the methods on this type while you're there. If you do, you may be able to remove the existing tests as they should be redundant with reasonably comprehensive doc-tests.

Alternatives

We can always leave this alone; it's not terrible as-is.

Wasmtime GitHub notifications bot (Feb 16 2023 at 18:59):

jameysharp labeled issue #5804:

Feature

The EntitySet type declared in cranelift/entity/src/set.rs is a fairly standard implementation of a bit-set. However, it uses 8-bit chunks in the EntitySet::elems storage vector. It should use a size closer to the width of a machine word.

Benefit

Using a larger size should give a small performance improvement.

Implementation

I'd prefer if elems were a Vec<usize>. Then the number of bits in each element is:

const BITS: usize = core::mem::size_of::<usize>() * 8

All the places which use the constant 8 should instead refer to BITS, and 7 should be replaced with BITS - 1.

If someone wants to tackle this, it would also be great if you could add doc-tests for the methods on this type while you're there. If you do, you may be able to remove the existing tests as they should be redundant with reasonably comprehensive doc-tests.

Alternatives

We can always leave this alone; it's not terrible as-is.

Wasmtime GitHub notifications bot (Mar 10 2023 at 10:37):

maekawatoshiki commented on issue #5804:

Hi, I want to work on this.

Wasmtime GitHub notifications bot (Mar 21 2023 at 19:39):

jameysharp closed issue #5804:

Feature

The EntitySet type declared in cranelift/entity/src/set.rs is a fairly standard implementation of a bit-set. However, it uses 8-bit chunks in the EntitySet::elems storage vector. It should use a size closer to the width of a machine word.

Benefit

Using a larger size should give a small performance improvement.

Implementation

I'd prefer if elems were a Vec<usize>. Then the number of bits in each element is:

const BITS: usize = core::mem::size_of::<usize>() * 8

All the places which use the constant 8 should instead refer to BITS, and 7 should be replaced with BITS - 1.

If someone wants to tackle this, it would also be great if you could add doc-tests for the methods on this type while you're there. If you do, you may be able to remove the existing tests as they should be redundant with reasonably comprehensive doc-tests.

Alternatives

We can always leave this alone; it's not terrible as-is.

Last updated: Dec 23 2024 at 12:05 UTC