Stream: git-wasmtime

Topic: wasmtime / PR #5978 cranelift-entity: more efficient `Ent...

Wasmtime GitHub notifications bot (Mar 10 2023 at 13:45):

maekawatoshiki opened PR #5978 from develop to main:

As discussed on https://github.com/bytecodealliance/wasmtime/issues/5804, this PR replaces u8 elements in EntitySet with usize elements.

I didn't add doc-tests. If I did so, I needed to add a piece of code like below to doc-tests for every method, which looks redundant to me. I think there's no need to remove the existing tests.
Let me know if something in my opinion is wrong.

// `EntityRef` impl for testing.
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
struct E(u32);

impl EntityRef for E {
    fn new(i: usize) -> Self {
        E(i as u32)
    }
    fn index(self) -> usize {
        self.0 as usize
    }
}

let mut m = EntitySet::new();
...

---

I realized that, in wasmtime/cranelift/entity, cargo test fails on some tests below.

running 52 tests
...omitted...

failures:

---- tests::basic_entity::cannot_construct_from_reserved_u32 stdout ----
note: test did not panic as expected
---- tests::basic_entity::cannot_construct_from_reserved_usize stdout ----
note: test did not panic as expected
---- tests::other_entity::cannot_construct_from_reserved_u32 stdout ----
note: test did not panic as expected
---- tests::other_entity::cannot_construct_from_reserved_usize stdout ----
note: test did not panic as expected
---- tests::prefix_entity::cannot_construct_from_reserved_u32 stdout ----
note: test did not panic as expected
---- tests::prefix_entity::cannot_construct_from_reserved_usize stdout ----
note: test did not panic as expected

failures:
    tests::basic_entity::cannot_construct_from_reserved_u32
    tests::basic_entity::cannot_construct_from_reserved_usize
    tests::other_entity::cannot_construct_from_reserved_u32
    tests::other_entity::cannot_construct_from_reserved_usize
    tests::prefix_entity::cannot_construct_from_reserved_u32
    tests::prefix_entity::cannot_construct_from_reserved_usize

test result: FAILED. 46 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Is this an expected behavior?

Wasmtime GitHub notifications bot (Mar 10 2023 at 20:54):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2023 at 20:54):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2023 at 20:54):

jameysharp created PR review comment:

While reviewing this patch I noticed that I'm not sure why the cardinality function was written the way it was. This is nothing wrong with your changes, but if you'd like to make another PR I think this function can be cleaned up.

As far as I can tell, self.len is supposed to always track the position of the last 1 bit in the set. If that's working correctly, then the first count_ones loop should work fine in all cases and we don't need a special case for the last element. The only trick is that instead of looping to self.len/BITS, we need to round up to the next element boundary with (self.len + (BITS - 1)) / BITS, like we've done elsewhere.

I don't want to make that change in this PR because it's possible that self.len is not being managed correctly and I'd want to think about that separately.

Wasmtime GitHub notifications bot (Mar 10 2023 at 20:54):

jameysharp created PR review comment:

I see a couple of places where variables have byte in their names, which is confusing now that they aren't actually bytes. Could you search for the word byte in this file and pick new names? In this case, I think byte_ix could be idx instead.

As future work, this loop could be replaced with something like self.elems[..self.len / BITS].iter().map(|x| x.count_ones() as usize).sum(). That should be faster by avoiding bounds checks. But I'd want that in a separate PR. I like that you've kept this PR simple, making only one change.

Wasmtime GitHub notifications bot (Mar 11 2023 at 04:37):

maekawatoshiki updated PR #5978 from develop to main.

Wasmtime GitHub notifications bot (Mar 11 2023 at 05:53):

maekawatoshiki requested jameysharp for a review on PR #5978.

Wasmtime GitHub notifications bot (Mar 13 2023 at 18:43):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 13 2023 at 19:22):

jameysharp merged PR #5978.

Last updated: Nov 22 2024 at 17:03 UTC