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

Encapsulate wasm execution context #428

Merged
merged 7 commits into from
Jun 9, 2021
Merged

Encapsulate wasm execution context #428

merged 7 commits into from
Jun 9, 2021

Conversation

Shillaker
Copy link
Collaborator

@Shillaker Shillaker commented Jun 8, 2021

To give intrinsic functions access to the executing module and call, we set them as global thread-local variables with global accessor methods.

Currently the logic for this is spread around the place and because it's handled with global variables, can end up affecting subsequent executions if not cleared up properly. It's also wrapped in a load of assertions because it's so fragile, and these assertions enforce needless restrictions (e.g. each thread can only ever set one executing call and not be changed).

In an attempt to tidy things up I've encapsulated this context into a class which hides the use of the local variables and uses RAII-like scoping, i.e. I can do something like:

// No execution context set

{
    WasmExecutionContext ctxA(moduleA, callA);
    // Execution context set to moduleA and callA
    
    { 
        WasmExecutionContext ctxB(moduleB, callB);
        // Execution context set to moduleB and callB
    }

    // Execution context set to moduleA and callA
}

// No execution context set

Unfortunately things rely on having access to a non-const Message object, so there's a bit of a cascade of non-const-ifying and I've had to add one unnecessary copy when resetting the context.

@Shillaker Shillaker self-assigned this Jun 8, 2021
@Shillaker Shillaker marked this pull request as ready for review June 8, 2021 10:44
Copy link
Collaborator

@csegarragonz csegarragonz left a comment

Choose a reason for hiding this comment

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

I don't see why we need non-const pointers? In fact, initialising them to null in the header file is AFAICT not very useful as we check the stack size and return null or not basing on that (not on the pointer being null). Thus, we could initialise a const pointer in the initialisation list.

Plus, I haven't seen (maybe I have missed) where we actually need the message to be non-const?

@@ -2,6 +2,7 @@
#include <faabric/util/bytes.h>
#include <faabric/util/macros.h>
#include <wamr/native.h>
#include <wasm/WasmExecutionContext.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this include necessary here? It's not used in the file elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, because the getExecutingCall/ getExecutingModule functions now live in this header rather than WasmModule.h

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right I had missed that 👍

#include <conf/FaasmConfig.h>
#include <wasm/WasmExecutionContext.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, just wondering, is this used here?

@Shillaker
Copy link
Collaborator Author

Shillaker commented Jun 9, 2021

I don't see why we need non-const pointers?

The existing interface (i.e. the old version of the getExecutingCall method) returned a non-const pointer, which is necessary because functions in the host interface need to modify the currently executing Message object (e.g. when setting output data). This means the pointer we hold in the WasmExecutionContext also has to be non-const, and so everything creating a context needs to provide a non-const pointer (hence the scattered edits).

I opted to do a message copy in the reset method to remove the const-ness to avoid having to propagate the change all the way into Faabric as well.

In fact, initialising them to null in the header file is AFAICT not very useful as we check the stack size and return null or not basing on that (not on the pointer being null). Thus, we could initialise a const pointer in the initialisation list.

Always initialising pointers to nullptr is just a style thing/ good practice. Admittedly in this case they will always be set by the constructor so it's probably unnecessary, however, I like to have it in place in case the code changes and someone forgets. We can remove it if you feel strongly though.

@Shillaker Shillaker merged commit c8443df into master Jun 9, 2021
@Shillaker Shillaker deleted the wasm-context branch June 9, 2021 07:54
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.

None yet

2 participants