Converted Memory into a readonly struct

[martindevans]

This is a demonstration of what's involved with converting Memory into a readonly struct, as discussed in (#219 (review)). As you can see this was almost no work. The bulk of the changes are fixing the tests.

My only concern with this is that it is now possible to call things on a default(Memory), which of course would have simply triggered a null reference exception before. I've checked through and I think it's safe in this case because everything uses store.Context which will trigger a null reference exception.

Thoughts?

[kpreisser]

I like it! (Maybe this could later also be done for other Extern types.)

One minor thing to note is that the Memory struct size (Unsafe.SizeOf<Memory>()) is currently 56 (on 64-bit) due to the fields Minimum, Maximum and Is64Bit, which can also be retrieved from the ExternMemory. Removing the fields would reduce the struct size to 24, in which case the properties would need to retrieve the values from the ExternMemory on every call. But I'm not sure how expensive that is, so it's probably OK to keep it in the struct.

Also note that because Memory implements an interface (IExternal), this will cause boxing to occur where this is used, for example with Linker.Define(string module, string name, object item), so an additional allocation will now occur there.
If desired, I think this could be avoided by having an internal Linker.Define(string, string, Store store, Extern extern), and then overloads for the struct types like Linker.Define(string, string, Memory) that calls it.

Thanks!

[martindevans]

this will cause boxing to occur where this is used, for example with Linker.Define(string module, string name, object item)

If it's just an internal interface I would change it to Linker.Define<T>(string module, string name, T item) where T : IExtern. Which would also have the benefit that you can't pass non-externs into it (which is currently checked at runtime). That won't work for the public API though since IExtern is internal. For the public API can we just add overloads for all the individual extern things, there are only 4 (Memory, Table, Global, Module) as far as I know?

[peterhuene]

For the public API can we just add overloads for all the individual extern things, there are only 4 (Memory, Table, Global, Module) as far as I know?

That sounds reasonable to me!

[martindevans]

MacOS CI failure is a bit cryptic, I'm hoping it's just another transient error.

[kpreisser]

Hi,
it seems you intended to rebase the PR on current main branch (resulting in 7e2d02a), but then merged it with the previous commit of this PR (617be6f), which is why the Github UI is now showing changes that are already part of main.

Can you reset the branch to commit 7e2d02a and force-push it (as I think that was the intended commit)?
Thanks!

[martindevans]

Thanks for the hint @kpreisser, I didn't notice that the history was in such a messy state! Doing as you suggested has fixed it.

[martindevans]

Unfortunately I don't have access to a Mac to investigate the test issues. There's no detail in the log on what the problem might be either :/

[kpreisser]

I'm also not sure why it's crashing, but maybe it is caused by/related to the MemoryAccessTests that allocate a 4 GB Memory (ItCanAccessMemoryWith65536Pages + ItThrowsForOutOfBoundsAccess).
Can you try to add the same attribue as for the Memory64AccessTests to skip them, to check if that avoids the crash? Though I'm not sure why this would happen on this PR only.

wasmtime-dotnet/tests/Memory64AccessTests.cs

Line 28 in 9562c78

[Fact(Skip = "Test consumes too much memory for CI")]

Thanks!

[martindevans]

After skipping some tests and commenting them back in one by one I've narrowed it down to this test:

[Fact]
public void AccessDefaultThrows()
{
    var memory = default(Memory);

    Assert.Throws<NullReferenceException>(() => memory.GetLength());
}

I added this test to reproduce the problem:

[Fact]
public void TestMacOsCI()
{
    try
    {
        object? o = null;
        o.Equals(o).Should().BeTrue();
    }
    catch (NullReferenceException)
    {
        return;
    }

    Assert.False(true);
}

This crashes the test runner!

[martindevans]

Superseded by #235, which allows Memory to remain as a class.