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

Optimize internal calls by passing only stack end for arguments #642

Draft
wants to merge 8 commits into
base: skip_nop_instructions
Choose a base branch
from

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Nov 16, 2020

Instead of passing the reference to the call arguments (a span on the top of the stack) just pass the pointer to the stack end.
Also, don't get the result by ExecutionResult, just expect the result being set to args[0] and the new stack end pointer to be returned.

By this change, the caller does not need to know what the type of the callee function is. See invoke_function() simplification. It only passes the stack pointer and updates the stack pointer after the call.

  1. This is a prototype, implementation is messy.
  2. The host functions are not updated to the new calling convention. So there is even more execute() wrapping currently. But they may and probably should be updated.
  3. I used "stack end", but also "stack top" can be used. The "stack end" looked easier and safer for prototype.
  4. Returning nullptr means trap, but this may be a trap for call without arguments where nullptr could be passed as the "no arguments" value. Currently the wrapper handles that nicely (see fake_arg). But we can consider some sentinel address values.
  5. It would be better to handle the imported function calls in the wrapper or in call implementation, not in the execute(). I realized this to late to change the design in the prototype.

@chfast chfast added the optimization Performance optimization label Nov 16, 2020
@chfast chfast requested review from axic and gumb0 November 16, 2020 16:58
@chfast chfast force-pushed the call_optimization_args_ptr branch from 986808c to 254e779 Compare November 17, 2020 10:54
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #642 (254e779) into skip_nop_instructions (2bdbbc5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                  Coverage Diff                   @@
##           skip_nop_instructions     #642   +/-   ##
======================================================
  Coverage                  98.36%   98.37%           
======================================================
  Files                         69       69           
  Lines                       9626     9668   +42     
======================================================
+ Hits                        9469     9511   +42     
  Misses                       157      157           

EXPECT_THAT(execute(parse(wasm), 1, {}), Result(1));
}

TEST(execute_call, call_void_with_one_argument)
Copy link
Member

Choose a reason for hiding this comment

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

@chfast I think these tests could be merged anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#641.

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

Successfully merging this pull request may close these issues.

2 participants