Add stats to WASM VMs. #9259

jplevyak · 2019-12-06T23:31:26Z

Signed-off-by: John Plevyak jplevyak@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add stats to WASM VMs including active and number created. Make V8 VMs clonable. Make the stats objects (Scope and StatNameSet) shared and with a separate lifetime from VMs to minimize memory usage and so that we can share them over VM reloads. The new VM stats were added because the VMs are destroyed only when the listener drains and those stats are useful to see how many are still around.
Risk Level: Low
Testing: Unit
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: John Plevyak <jplevyak@gmail.com>

htuch · 2019-12-10T23:28:20Z

@jmarantz can you take a pass on this, given your deep stats knowledge and recent discussions with @jplevyak on this subject?

jmarantz · 2019-12-11T03:34:18Z

hi -- can you give some more context to this PR? I'd like to understand better the relationship between threads & VM instances, and what exactly is the semantics when you clone a VM.

Thanks!

/wait

jplevyak · 2019-12-11T04:43:54Z

As per our discussions, the stats are going to be initialized infrequently should outlive the restarted VMs so that creation costs can be amortized and so that they do not get reset (to zero for counters). This PR decouples the lifetime of stats from listeners and VM and also adds individual stats for active VMs and created VMs providing insight into VM life cycles. A coming PR will use these mechanisms to add stats for VM startup issues as the current xDS configuration mechanism which is used for all filter/listener configurations (not just WASM) makes a configuration active even if there are issues (e.g. exceptions) and these stats will improve visibility of configuration issues (i.e. they can be be alerted on).

jplevyak · 2019-12-11T04:47:32Z

@mandarjog and @kyessenov have context on this issue

kyessenov · 2019-12-11T17:41:27Z

The particular issue we were debugging was accumulation of Wasm VMs attached to draining listeners. Since draining listeners are removed on a timer basis, we may have quite a number of VMs alive at any point (including their onTick callbacks). A counter on the number of active VMs would help us here.

jmarantz · 2019-12-11T17:46:50Z

very cool. The comments from @jplevyak and @kyessenov would both be appropriate for the PR description.

jplevyak · 2019-12-12T00:41:58Z

Updated PR description. PTAL

jmarantz

A few nits, mostly around style nits and doc.

There's a bunch going on here that I think @lizan will need to review, or someone else that's read the book of knowledge of wasm :)

bazel/external/wee8.genrule_cmd

source/extensions/common/wasm/null/null_vm.cc

source/extensions/common/wasm/null/null_vm.h

source/extensions/common/wasm/wasm_vm.h

source/extensions/common/wasm/wasm_vm_base.h

Signed-off-by: John Plevyak <jplevyak@gmail.com>

source/extensions/common/wasm/null/null_vm.cc

Signed-off-by: John Plevyak <jplevyak@gmail.com>

source/extensions/common/wasm/null/null_vm.cc

source/extensions/common/wasm/wasm_vm_base.h

Signed-off-by: John Plevyak <jplevyak@gmail.com>

source/extensions/common/wasm/wasm_vm_base.h

mattklein123

LGTM with some small comments, thanks.

/wait

source/extensions/common/wasm/wasm_vm.h

mattklein123 · 2019-12-13T19:28:41Z

source/extensions/common/wasm/wasm_vm.h

@@ -287,7 +291,7 @@ struct SaveRestoreContext {
 };

 // Create a new low-level WASM VM using runtime of the given type (e.g. "envoy.wasm.runtime.wavm").
-WasmVmPtr createWasmVm(absl::string_view runtime);
+WasmVmPtr createWasmVm(absl::string_view runtime, Stats::ScopeSharedPtr scope);


nit: pass either by const-ref or by explicit && all the way through for clarity.

FYI: I checked the compiler output and there is no performance benefit to using std::move + && if the variable goes out of scope immediately as the compiler does copy elision. Since the pointer is shared there is not ownership being passed so I don't understand what you mean by "clarity" here as we typically pass with && to indicate that there is a passing of ownership which is not the case here. However, taking your suggestions:

Using && means that the caller will have to make an explicit copy if the variable is still in use after the call or std::move if it is not which clutters up the code and adds an extra burden as the caller who now has to manage the life cycle of what is logically a shared object. If the caller decides to take the easy way out and always make a copy, the code will be less efficient as the copy is explicit and can't be removed via copy elision by the compiler.

Switching to const-ref. That should produce equally efficient code and adds no burden to the caller, although because there is now non-local sharing (i.e. the variable in the caller is now aliased into the callees) it is nominally less safe (an indirect function call from the callee which causes the caller to clear or or replace that variable could effect the variable in the callee since it is a reference). Personally I think it would be better to pass by value here since it is less code, just as efficient and safer (no non-local sharing).

All that said, I switched it to const-ref since that seems to be the more common idiom in Envoy when dealing with shared objects and no passing of ownership.

PTAL

source/extensions/common/wasm/wasm_vm_base.h

Signed-off-by: John Plevyak <jplevyak@gmail.com>

…m-stats-store

Signed-off-by: John Plevyak <jplevyak@gmail.com>

stale · 2020-01-04T23:33:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jplevyak · 2020-01-06T18:08:02Z

Ping. This PR is largely independent of the Architecture doc and I have answered all the comments.

PTAL

mattklein123

LGTM with small nits, thanks.

/wait

mattklein123 · 2020-01-06T18:21:41Z

source/extensions/common/wasm/v8/v8.cc

@@ -247,9 +249,28 @@ bool V8::load(const std::string& code, bool /* allow_precompiled */) {
  ::memcpy(source_.get(), code.data(), code.size());

  module_ = wasm::Module::make(store_.get(), source_);
+  if (module_) {
+    shared_module_ = module_->share();
+    RELEASE_ASSERT(shared_module_ != nullptr, "");


nit: in general we don't release ASSERT things like this since it shouldn't fail and will crash in obvious ways. Is there any reason to do this here and elsewhere?

mattklein123 · 2020-01-06T18:22:30Z

source/extensions/common/wasm/null/null_vm.cc

@@ -7,6 +7,7 @@
 #include "envoy/registry/registry.h"

 #include "common/common/assert.h"
+#include "common/singleton/threadsafe_singleton.h"


Is this still needed? Same elsewhere.

Removed. Done.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

mattklein123

Thanks!

Add stats to WASM VMs.

36dcb98

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak mentioned this pull request Dec 6, 2019

Upstream ABI Part 1 (logging + stats) from envoy-wasm . #9217

Closed

mattklein123 assigned lizan, mattklein123 and htuch Dec 7, 2019

htuch requested a review from jmarantz December 10, 2019 23:27

htuch added the waiting:any label Dec 10, 2019

jmarantz self-assigned this Dec 11, 2019

repokitteh-read-only bot added waiting and removed waiting:any labels Dec 11, 2019

jmarantz reviewed Dec 12, 2019

View reviewed changes

Convert Singleton to a global.

0ba0bdf

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak requested a review from lizan as a code owner December 12, 2019 21:50

repokitteh-read-only bot removed the waiting label Dec 12, 2019

jmarantz reviewed Dec 12, 2019

View reviewed changes

source/extensions/common/wasm/null/null_vm.cc Outdated Show resolved Hide resolved

Address coments.

80d36f2

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jmarantz reviewed Dec 12, 2019

View reviewed changes

source/extensions/common/wasm/null/null_vm.cc Outdated Show resolved Hide resolved

source/extensions/common/wasm/wasm_vm_base.h Outdated Show resolved Hide resolved

Address comments.

766c7d9

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jmarantz previously approved these changes Dec 13, 2019

View reviewed changes

source/extensions/common/wasm/wasm_vm_base.h Outdated Show resolved Hide resolved

source/extensions/common/wasm/wasm_vm_base.h Outdated Show resolved Hide resolved

mattklein123 requested changes Dec 13, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 13, 2019

Address comments.

bfefaf4

Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak dismissed jmarantz’s stale review via bfefaf4 December 18, 2019 18:48

repokitteh-read-only bot removed the waiting label Dec 18, 2019

jplevyak requested a review from jmarantz December 18, 2019 18:50

jplevyak mentioned this pull request Dec 18, 2019

Update Wasm VM code to the latest from envoy-wasm. #9077

Closed

Fix pedantic spelling.

a3d8030

Signed-off-by: John Plevyak <jplevyak@gmail.com>

mattklein123 added the waiting label Dec 20, 2019

Address comments.

48bfc76

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label Dec 23, 2019

jplevyak mentioned this pull request Dec 23, 2019

Add end user documentation for WASM extension. #9462

Open

jplevyak requested a review from mattklein123 December 23, 2019 18:53

htuch removed their assignment Dec 23, 2019

jplevyak added 6 commits December 28, 2019 17:55

Address comments.

66cdce0

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comments.

4069f42

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comments.

3374ab3

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Address comments.

963f3b1

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into was…

d721528

…m-stats-store

Address comments.

bb8dfc0

Signed-off-by: John Plevyak <jplevyak@gmail.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 4, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 6, 2020

mattklein123 requested changes Jan 6, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 6, 2020

Address comments.

4ec4232

Signed-off-by: John Plevyak <jplevyak@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 9, 2020

mattklein123 approved these changes Jan 10, 2020

View reviewed changes

mattklein123 merged commit 079af3a into envoyproxy:master Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stats to WASM VMs. #9259

Add stats to WASM VMs. #9259

jplevyak commented Dec 6, 2019 •

edited

Loading

htuch commented Dec 10, 2019

jmarantz commented Dec 11, 2019

jplevyak commented Dec 11, 2019

jplevyak commented Dec 11, 2019

kyessenov commented Dec 11, 2019

jmarantz commented Dec 11, 2019 •

edited

Loading

jplevyak commented Dec 12, 2019

jmarantz left a comment

mattklein123 left a comment

mattklein123 Dec 13, 2019

jplevyak Dec 23, 2019

stale bot commented Jan 4, 2020

jplevyak commented Jan 6, 2020

mattklein123 left a comment

mattklein123 Jan 6, 2020

jplevyak Jan 9, 2020

mattklein123 Jan 6, 2020

jplevyak Jan 9, 2020

mattklein123 left a comment

Add stats to WASM VMs. #9259

Add stats to WASM VMs. #9259

Conversation

jplevyak commented Dec 6, 2019 • edited Loading

htuch commented Dec 10, 2019

jmarantz commented Dec 11, 2019

jplevyak commented Dec 11, 2019

jplevyak commented Dec 11, 2019

kyessenov commented Dec 11, 2019

jmarantz commented Dec 11, 2019 • edited Loading

jplevyak commented Dec 12, 2019

jmarantz left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Dec 13, 2019

Choose a reason for hiding this comment

jplevyak Dec 23, 2019

Choose a reason for hiding this comment

stale bot commented Jan 4, 2020

jplevyak commented Jan 6, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Jan 6, 2020

Choose a reason for hiding this comment

jplevyak Jan 9, 2020

Choose a reason for hiding this comment

mattklein123 Jan 6, 2020

Choose a reason for hiding this comment

jplevyak Jan 9, 2020

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

jplevyak commented Dec 6, 2019 •

edited

Loading

jmarantz commented Dec 11, 2019 •

edited

Loading