Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wiring SharedMemory with it's corresponding bindings #255

Merged
merged 10 commits into from
Nov 25, 2024

Conversation

atilag
Copy link
Contributor

@atilag atilag commented Oct 23, 2024

I think this is more or less what we need to have an initial support for SharedMemory.
I'll be adding tests and examples if you think I'm heading into the right direction.

TODO:

  • Add tests for Shared Memory
  • Adapt the rest of the bindings to the new c-api changes

@atilag atilag marked this pull request as draft October 23, 2024 02:18
wasmtime/_sharedmemory.py Outdated Show resolved Hide resolved
wasmtime/_sharedmemory.py Outdated Show resolved Hide resolved
@atilag
Copy link
Contributor Author

atilag commented Oct 23, 2024

I have a "test case" with this code:

        import wasmtime

        engine = wasmtime.Engine()

        wasi_config = wasmtime.WasiConfig()
        wasi_config.inherit_stdout()

        store = wasmtime.Store(engine)
        store.set_wasi(wasi_config)

        wasm_file = 'my_wasi_program.wasm'
        with open(wasm_file, 'rb') as f:
            wasm_bytes = f.read()

        module = wasmtime.Module(engine, wasm_bytes)

        linker = wasmtime.Linker(engine)
        linker.define_wasi()

        memory_type = wasmtime.MemoryType(wasmtime.Limits(0, 4000), shared=True) # I have added `shared` here as well
        memory = wasmtime.SharedMemory(engine, memory_type)

      
        linker.define(store, "env", "memory", memory) 


        instance = linker.instantiate(store, module)
        ...

This is throwing an error coming from the wasmtime runtime:

ty = <wasmtime._types.MemoryType object at 0x101e30580>

    def __init__(self, engine: Engine, ty: MemoryType):
        """
        Creates a new shared memory in `store` with the given `ty`
        """

        sharedmemory = ffi.wasmtime_sharedmemory_t()
        ptr = POINTER(ffi.wasmtime_sharedmemory_t)()
        error = ffi.wasmtime_sharedmemory_new(engine.ptr(), ty.ptr(), byref(ptr))
        if error:
>           raise WasmtimeError._from_ptr(error)
E           wasmtime._error.WasmtimeError: shared memory must have the `shared` flag enabled on its memory type

which kind of make sense, because there's no support for the "shared" flag in the wasmtime_memorytype_new exported in the C-API. I also have a patch there (will PR soon), and made some progress, but then a clone() of the shared memory type is panicking, so still trying to figure out why.

wasmtime/_bindings.py Outdated Show resolved Hide resolved
wasmtime/_ffi.py Show resolved Hide resolved
wasmtime/_sharedmemory.py Outdated Show resolved Hide resolved
wasmtime/_sharedmemory.py Outdated Show resolved Hide resolved
wasmtime/_sharedmemory.py Outdated Show resolved Hide resolved
wasmtime/_sharedmemory.py Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member

Looks good to me! I think the lack of read/write on the Rust side of things is probably an oversight and so might be worth adding here eventually, but I think it's nonetheless reasonable to start here. Before landing this'll want to have a few smoke tests too to ensure that everything is wired up right

@atilag
Copy link
Contributor Author

atilag commented Oct 25, 2024

Do you know how can I generate the _bindings.py file out of the wasmtime C-API? I have the latter cloned, modified to add the 5th argument (bool shared) to the MemoryType, and I'm able to build and generate the .dlyb (on my mac).

@alexcrichton
Copy link
Member

It's unfortunately not easy at this time. Basically ./ci/download-wasmtime.py will place all the headers/etc into a location and what you'll want to do is overwrite/link the in-tree headers in the same location. From there you can also replace the *.dylib with what you built locally, and that way you can test locally.

@atilag
Copy link
Contributor Author

atilag commented Nov 5, 2024

Ok, I'm resuming this, now that we have all the pieces in place for the wasmtime runtime.

@alexcrichton
Copy link
Member

Sounds good! As a heads up CI here pulls from a release branch of Wasmtime but you should be able to pull it from the "dev" release which should have all the latest changes.

@atilag
Copy link
Contributor Author

atilag commented Nov 12, 2024

After pulling lastest changes from the c-api and regenerating the bindings, I realized that I need to integrate other changes not related to the memory as well (because regenerating the bindings pull all the changes, of course). This is fine, I can do it here as well, I wouldn't expect the PR to grow a lot.

@alexcrichton
Copy link
Member

Ah yeah no worries, happy to review that here too!

@atilag
Copy link
Contributor Author

atilag commented Nov 14, 2024

Ok, there's a test failing but I just saw that this issue and this branch from @jder is fixing what is missing here.
I was going to suggest merging this (after I add some tests ), and maybe, disabling the failing test?, until the work from @jder is merged.

@alexcrichton
Copy link
Member

Feel free to hack around things here and/or split out the update to Wasmtime 27. I'm busy this week but I can more thoroughly dig in next week

@alexcrichton
Copy link
Member

The Wasmtime version in use was updated in #257 so if you want to rebase that should pull in all the necessary updated bindings I think

atilag and others added 7 commits November 18, 2024 20:38
implemented in the Rust API for wasmtime.
A SharedMemory is not associated to any Store, and the read/write
methods from the linear memory seem  to rely on the Store in order
to properly operate.
and adjusting the shared memory arguments to match the new api, I
found out that I needed to adjust other parts of the bindings that I guess
have changed since the last release.
In this commit, I only change the (shared) memory related ones, but I
think I can adjust the rest of the c-api that has changed as well in
the following commits.
changes in the C-APO from Wasmtime runtime repo
@atilag
Copy link
Contributor Author

atilag commented Nov 19, 2024

I'm having a Seg. Fault after writing a test, still debugging it (been busy this past weeks!). Once I have fixed it, I'll update the branch and I think we should be ok to merge this.

@alexcrichton
Copy link
Member

It looks like mypy is reporting some type errors which might be the reason for the faults? If mypy passes it may be easier to debug perhaps

@alexcrichton
Copy link
Member

As a heads up I'm going to publish 27.0.0 in #258 but happy to do a point release with this when it's ready.

* Adding some unit tests for shared memory
@atilag atilag marked this pull request as ready for review November 25, 2024 01:41
@atilag
Copy link
Contributor Author

atilag commented Nov 25, 2024

Yep, mypy has indeed captured all the problems.
I added some unit tests too. I used the same testing structure that exists around Memory, it's a good start but maybe looks insuficient. Will try to add some acceptance tests in another follow up PR, with the intention to merge this asap if you find it ready.
Thanks Alex!

@alexcrichton
Copy link
Member

Looks good to me! Could you go ahead and add the tests here? I'd like to make sure to at least exercise some basics in CI even if more advanced testing is coming later

@atilag
Copy link
Contributor Author

atilag commented Nov 25, 2024

Of course that I forgot to git add the test file xDDD
Done!

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for this! Would you like a publish of 27.0.2 for this?

@alexcrichton alexcrichton merged commit aee787c into bytecodealliance:main Nov 25, 2024
12 checks passed
@atilag
Copy link
Contributor Author

atilag commented Nov 25, 2024

I don't have a "production" use-case for this just yet, we are investigating the potential fit for some use-cases we have, but releasing would make things a little bit easier moving forward, so it's up to you :)

@alexcrichton
Copy link
Member

Sounds good! I'll do that in #261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants