Tracing improvements #50

mganandraj · 2021-04-01T08:12:30Z

This PR makes the following improvement to the ETW tracing,

All traces are now correctly attributed to the right runtime.
Fixes crash when multiple runtimes are registering the ETW provider.
Enables overriding some arguments through system registry.
Improvements to the powershell script to make the tracing easier.

I reformatted some of the files accidentally. I can't find an easy way to revert those formatting hcanges.

Microsoft Reviewers: Open in CodeFlow

src/public/V8JsiRuntime.h

tudorms · 2021-04-02T17:28:24Z

Please also bump the version in config.json

…the version

…too many commits.. I seem to be on a hurry)

vmoroz · 2021-04-02T19:44:56Z

src/V8JsiRuntime.cpp

 #include <atomic>
 #include <cstdlib>
 #include <list>
 #include <mutex>
 #include <sstream>

+#include "V8JsiRuntime_impl.h"


We typically want the main .h file to be the first one. It is also better to put all standard headers after custom header - this way we can partially ensure that custom headers have all required includes.

vmoroz · 2021-04-02T19:48:39Z

It is difficult to review changes because of the formatting changes in the same PR.
Please split the PR into two different PRs:

The first one one will be the formatting changes only. Note that the code is not foormatted using the clang format settings that we have. Open the folder in VS Code and format there - it should pickup the Clang format settings.
Then after the formatting change is merged - reformat your code using Clang format, and merge the previous PR. - After that we should see only your changes.

vmoroz · 2021-04-02T19:50:54Z

src/V8Platform.h

@@ -28,52 +28,37 @@ class ETWTracingController : public v8::TracingController {
  ETWTracingController(bool enabled) : enabled_(enabled) {}
  ~ETWTracingController() = default;

-  const uint8_t *GetCategoryGroupEnabled(const char *category_group) override;
+  const uint8_t* GetCategoryGroupEnabled(const char* category_group) override;


const char* [](start = 41, length = 12)

Our clang format settings put the star on the right. What tool do you use to format code?

vmoroz · 2021-04-02T19:56:01Z

src/V8JsiRuntime.cpp

@@ -1574,12 +1572,54 @@ v8::Local<v8::Value> V8Runtime::valueRef(const jsi::Value &value) {
  }
 }

-std::unique_ptr<jsi::Runtime> makeV8Runtime(V8RuntimeArgs &&args) {
+void applyOverrides(V8RuntimeArgs& args) {


applyOverrides [](start = 5, length = 14)

It would be great to avoid this DLL accessing the registry. Can it be the application that does it?
The V8JSI is an open source project and it seems to be wrong to use MS Office key in the path and have a "backdoor" affecting any app in the system that uses this DLL.

I do partially agree Vladimir .. but having such control will be extremely helpful when building debugging and performance tooling. We can hopefully remove these once our tooling is matured. I'm removing the "Office" subpaths from the registry key.. Hope it works for you.

Can we have a subkey per application? I.e. per executable file name that uses the V8JSI DLL.
E.g. L"SOFTWARE\Microsoft\V8JSI\foo.exe";

In reply to: 606578849 [](ancestors = 606578849)

Why is this needed at all? These values are already being passed through the arguments; the consumer is already capable of passing them in.

In reply to: 606610175 [](ancestors = 606610175,606578849)

tudorms · 2021-04-02T21:18:43Z

Alternatively just reformat the code in vscode / clang format and push a PR interation here.

In reply to: 812684252 [](ancestors = 812684252)

mganandraj · 2021-04-03T01:44:41Z

It is difficult to review changes because of the formatting changes in the same PR.
Please split the PR into two different PRs:

The first one one will be the formatting changes only. Note that the code is not foormatted using the clang format settings that we have. Open the folder in VS Code and format there - it should pickup the Clang format settings.

Then after the formatting change is merged - reformat your code using Clang format, and merge the previous PR. - After that we should see only your changes.

The code got (accidentally) formatted by VS using chromium formatting rules .. I realized the formatting changes after making a bunch of changes on top, and attempted reverting but it turned out to be very messy ..

I don't have strong opinions on the formatting rules. When we started this codebase, we roughly followed the V8 formatting rules for things such as the variable, function naming, which still stays. As we build the code alongside V8, and devs (incl. me) usually create one big VS solution of V8 with this JSI code as a subproject, the chromium formatting rules fits the best i think. But, that is a very subjective discussion which I don't want to get into.

I'm reformatting the code with VS code using our checked in clang format as @tudorms suggested which is the least expensive resolution. Hope it works for you.

@tudorms

…istry path

vmoroz · 2021-04-03T03:25:22Z

src/V8JsiRuntime.cpp

@@ -28,7 +26,9 @@ using namespace facebook;

 namespace v8runtime {

-thread_local uint16_t V8Runtime::tls_isolate_usage_counter_ = 0;
+thread_local uint16_t V8Runtime::tls_isolate_usage_counter_{0};
+thread_local std::weak_ptr<V8Runtime::CounterStores>


weak_ptr [](start = 18, length = 8)

Ideally we should stop using TLS. In future we want to be able to use The V8 engine in scenarios without a dedicated JS thread. A better approach is to assciate the data with the Runtime, V8 context, or V8 isolate.
Anyway, there is no mechanism to call destructor on the TLS stored objects. Please reconsider having std::weak_ptr as TLS value. TLS is usually good only for simple values such as numbers or raw pointers.

I looked through the code and still do not understand why we cannot associate counters with the Runtime or Isolate instead of TLS.

In reply to: 606609468 [](ancestors = 606609468)

We need to access the counters from the static callback functions that we register to the isolate .. Essentially, what we want is access to "this" runtime pointer in the static callbacks .. The access to counters starts even before the isolate is properly constructed, hence we can't use the isolate data slots ..

I was also hesistant to add tls data .. I initially had a unique instance id counter to uniquely identify an instance, instead of a thread id .. But it turned out to be a lot more code and uglier. .. As V8 isolates and a thread is 1:1 mapped, I think the tls is fine for our scenario.. Essentialy, we can treat the counters as per isolate counters, not really per-runtime counters ..

We are not really gaining anything by having the weak_ptr .. even a raw pointer in tls with the shared_ptr member for lifetime manager suffice for us .. But, i made this change to weak_ptr just before the CR to make the code look nicer :) the naked pointer looks bad .. But as you suggested this, i'll revert the tls entry to a nake pointer.

vmoroz · 2021-04-03T04:30:41Z

src/V8JsiRuntime.cpp

@@ -572,19 +589,22 @@ void V8Runtime::initializeV8() {
 }

 V8Runtime::V8Runtime(V8RuntimeArgs &&args) : args_(std::move(args)) {
+  counter_stores_ = std::make_shared<CounterStores>();
+  tls_counter_stores_ = counter_stores_;


Why do we create them in two places: here in constructor and in the CreateNewIsolate() method? In most cases the CreateNewIsolate will override values created here.

Ah .. This was a mistake .. Thanks !

tudorms · 2021-04-15T18:05:08Z

src/V8JsiRuntime.cpp

 std::unique_ptr<jsi::Runtime> makeV8Runtime(V8RuntimeArgs &&args) {
+  applyOverrides(args);


applyOverrides(args);

Can you submit the rest of the changes besides this registry functionality (I think this is the only sticking point of the PR and we can revisit it later). There's a formatting PR in the pipeline and merging that would be painful after the fact.

Tracing improvements

05bc2dc

mganandraj requested a review from tudorms April 1, 2021 08:12

Merge remote-tracking branch 'ms/master' into TracingImprovements

695e7e1

mganandraj requested a review from a team April 2, 2021 15:21

tudorms reviewed Apr 2, 2021

View reviewed changes

src/public/V8JsiRuntime.h Show resolved Hide resolved

mganandraj added 3 commits April 2, 2021 10:53

Keeping the ABI layout by reintroducing the config options & bumping …

8b07574

…the version

Removign duplicate fields

7539792

More changes to make the config option order unchanged .. (Sorry for …

679c658

…too many commits.. I seem to be on a hurry)

vmoroz reviewed Apr 2, 2021

View reviewed changes

Reformatting the code using our clang format rules and fixing the reg…

4e3e6e3

…istry path

vmoroz reviewed Apr 3, 2021

View reviewed changes

tudorms reviewed Apr 15, 2021

View reviewed changes

shivenmian self-assigned this Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing improvements #50

Tracing improvements #50

mganandraj commented Apr 1, 2021 •

edited by ghost

Loading

tudorms commented Apr 2, 2021

vmoroz Apr 2, 2021

vmoroz commented Apr 2, 2021

vmoroz Apr 2, 2021

vmoroz Apr 2, 2021

mganandraj Apr 3, 2021

vmoroz Apr 3, 2021 •

edited

Loading

tudorms Apr 5, 2021

tudorms commented Apr 2, 2021

mganandraj commented Apr 3, 2021

vmoroz Apr 3, 2021

vmoroz Apr 3, 2021 •

edited

Loading

mganandraj Apr 3, 2021

vmoroz Apr 3, 2021

mganandraj Apr 3, 2021

tudorms Apr 15, 2021

		std::unique_ptr<jsi::Runtime> makeV8Runtime(V8RuntimeArgs &&args) {
		applyOverrides(args);

Tracing improvements #50

Are you sure you want to change the base?

Tracing improvements #50

Conversation

mganandraj commented Apr 1, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

tudorms commented Apr 2, 2021

Choose a reason for hiding this comment

vmoroz commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmoroz Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudorms commented Apr 2, 2021

mganandraj commented Apr 3, 2021

Choose a reason for hiding this comment

vmoroz Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mganandraj commented Apr 1, 2021 •

edited by ghost

Loading

vmoroz Apr 3, 2021 •

edited

Loading

vmoroz Apr 3, 2021 •

edited

Loading