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.

Sign up for GitHub

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

Jump to bottom

wasm: add V8-based WebAssembly runtime. #8592

Merged
merged 25 commits into from
Nov 5, 2019
Merged

wasm: add V8-based WebAssembly runtime. #8592

merged 25 commits into from
Nov 5, 2019

Conversation

PiotrSikora
Copy link
Contributor

Part of #4272.

Signed-off-by: Piotr Sikora piotrsikora@google.com

@PiotrSikora
wasm: add V8-based WebAssembly runtime.
7efa4c9 
Part of envoyproxy#4272.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: fix clang-tidy errors.
4e1281a 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora marked this pull request as ready for review October 12, 2019 10:25
@PiotrSikora
Copy link
Contributor Author

Coverage isn't great, but should be ready for review otherwise.

cc @mattklein123 @lizan @jplevyak

@mattklein123 mattklein123 self-assigned this Oct 13, 2019
@PiotrSikora
Copy link
Contributor Author

macOS builds are failing because of an old XCode version on Azure Pipelines (while most of the bleeding edge compiler flags are disabled, XCode 10.x is based on clang-7, which is a bit too old for Chromium/V8, which normally bundles clang from trunk).

I tried to configure Azure Pipelines to use XCode 11, but I cannot figure out the correct value for xcWorkspacePath. Luckily, it looks that the macOS images on Azure Pipelines will switch to XCode 11.1 by default in a week or two.

@PiotrSikora
review: use XCode 11.
e7d91ff 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: use XCode 11 (take 2).
21d96ad 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

You'll need to run xcode-select: https://docs.microsoft.com/en-us/azure/devops/pipelines/agents/hosted?view=azure-devops#xcode

Thanks! That was way easier than using XCode task.

mattklein123
mattklein123 reviewed Oct 17, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Cool stuff. Will defer to @lizan for review since this is mostly build setup. Thank you!

@mattklein123 mattklein123 removed their assignment Oct 17, 2019
lizan
lizan reviewed Oct 24, 2019
View reviewed changes
source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved
.azure-pipelines/macos.yml Outdated Show resolved Hide resolved
bazel/external/wee8.BUILD
@@ -0,0 +1,24 @@
licenses(["notice"]) # Apache 2
Copy link
Member

Choose a reason for hiding this comment

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

I thought you're working on bazel native build, where is it? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am, but it's not my top priority at this moment.

source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved
source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved
@lizan
Copy link
Member

lizan commented Oct 24, 2019

@mattklein123 FYI the v8.cc (whose diff is automatically hidden by GH 🤦‍♂ ) is not build setup and that's the crux of this PR, do you want to take a look too?

@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm-v8
26669cb 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: XCode -> Xcode.
ca8eea0 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: add trailing underscore to struct members.
0a5da84 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: remove [wasm] prefix from the logs.
1076dc6 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: use WasmVmPtr.
cafcff8 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: add few more tests.
00c6018 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: fix spelling.
ddef62a 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
lizan
lizan reviewed Oct 25, 2019
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

mostly style, are you adding tests for more coverage?

source/extensions/common/wasm/v8/v8.h Outdated Show resolved Hide resolved
source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved
source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved
source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved
source/extensions/common/wasm/v8/v8.cc Show resolved Hide resolved
@PiotrSikora
review: add more tests.
615df77 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: constexpr.
53b8237 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: drop leading underscore.
9aaca5f 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: use WasmVmPtr.
7fe2bd8 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm-v8
c36eb95
@PiotrSikora
review: limit the number of function types used in tests.
b28a272 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: test getMemoryOffset.
3e3c10e 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: test function failure.
7c833a7 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: more tests.
63c1b21 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@lizan
Copy link
Member

lizan commented Oct 28, 2019

@PiotrSikora v8.cc still lacks some coverage, can you improve it more? https://293497-65214191-gh.circle-artifacts.com/0/coverage/coverage/proc/self/cwd/source/extensions/common/wasm/v8/v8.cc.html

@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm-v8
34c33e7 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: improve coverage.
6505b6e 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@lizan I've added more tests and ripped out dead code, the rest is a bunch of logging helpers and logs for stuff that we don't have too much use right now... let me know if the coverage looks reasonable enough now.

@PiotrSikora
Copy link
Contributor Author

/azp run envoy-linux

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 8592 in repo envoyproxy/envoy

@PiotrSikora PiotrSikora mentioned this pull request Nov 1, 2019
Merged
@lizan
Copy link
Member

lizan commented Nov 1, 2019

@PiotrSikora can you merge master?

@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm-v8
c90f90a
@PiotrSikora
review: update wee8 to 7.9.317.14.
c09e8d1 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: Xcode 11.1 is now the default.
04c3a2c 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@lizan ping

@lizan lizan merged commit 6bbe6f3 into envoyproxy:master Nov 5, 2019
@moderation
Copy link
Contributor

@PiotrSikora Very cool to see this capability get closer. Are there plans to add to https://github.com/envoyproxy/envoy/blob/master/source/extensions/extensions_build_config.bzl so WASM can be compiled out if not required?

@lizan
Copy link
Member

lizan commented Nov 5, 2019

@moderation all wasm code are not linked into release binary yet. When we do extension registration it will be added to extensions_build_config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@lizan lizan lizan approved these changes

Assignees

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PiotrSikora @lizan @moderation @mattklein123