This repository has been archived by the owner. It is now read-only.

Windows: Add NT Perf Counter instrumentation to Node #4285

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
4 participants

sblom commented Nov 15, 2012

The idea here is to report high-level Node.exe activity metrics in a way that Windows admin tooling expects to collect--i.e. using perfmon.exe

node.gyp
+ }],
+ # if not perfctr add perfctr_macros.py to inputs
+ [ 'node_use_perfctr=="false"', {
+ 'inputs': [ 'src/perfctr_macros.py' ]
@sblom

sblom Nov 15, 2012

I actually want to fix how this part and the previous (DTrace/ETW) part interplay. Don't read too closely for now. I expect to do something like the FIXME (in red) suggests, just haven't quite figured it out yet.

@piscisaureus

piscisaureus Nov 15, 2012

Member

ok, i'll wait for it

@sblom

sblom Nov 17, 2012

much better now

.gitignore
@@ -31,6 +31,8 @@ _UpgradeReport_Files/
ipch/
*.sdf
*.opensdf
+src/res/node_perfctr_provider*.*
@piscisaureus

piscisaureus Nov 15, 2012

Member

Why do the "unimportant" files not go into <(SHARED_INTERMEDIATE_DIR) ?

+ );
+
+
+HMODULE advapimod;
@piscisaureus

piscisaureus Nov 15, 2012

Member

what was the reason to use run-time module loading instead of linking to advapi32.lib ?

@sblom

sblom Nov 16, 2012

Good question. From what I understand, the idea is that this allows node.exe to run on Windows XP when some of these APIs aren't available. I'll do the legwork to make sure we actually have that problem here.

For the ETW code, we did; less sure for perf counters which existed even in the very early WinNT days.

@sblom

sblom Nov 16, 2012

Confirmed that these APIs are not all present on Windows XP so LoadLibrary/GetProcAddress are the right way to go (same pattern was used for ETW).

configure
+ # By default, enable Performance counters on Windows.
+ if sys.platform.startswith('win32'):
+ o['variables']['node_use_perfctr'] = b(not options.without_perfctr);
+ elif b(options.with_perfctr) == 'true':
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

This could be simplified to elif options.with_perfctr: but it seems the ETW check uses the same pattern.

@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Actually, this could be simplified to a one-liner:

o['variables']['node_use_perfctr'] =
  b(sys.platform.startswith('win32') and not options.without_perfctr or options.with_perfctr)
@piscisaureus

piscisaureus Nov 16, 2012

Member

And nobody will understand except superpythonistas

@sblom

sblom Nov 17, 2012

Both the ETW and Perf Counters implementations got largely copied from the DTrace one. Granted I guess ETW and perfctrs is a little simpler.

Getting rid of the extraneous b() == 'true' seems like a clear win (committed, pushed), but the one-liner less so.

node.gyp
+ 'outputs': [
+ 'src/res/node_perfctr_provider.rc',
+ 'src/res/node_perfctr_provider.h',
+ 'src/res/node_perfctr_provider_r.h'
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

I think @piscisaureus already pointed it out but these should go into SHARED_INTERMEDIATE_DIR.

+
+
+static uint64_t counter_gc_start_time;
+static uint64_t counter_gc_end_time;
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

#include <stdint.h> if you use uint64_t, don't rely on other headers to pull it in.

@sblom

sblom Nov 17, 2012

Where's our favorite place to get that? Looks like it's currently coming in by way of uv.h--are you suggesting that I explicitly include uv.h around line 22? I'd include stdint.h directly, but it looks like uv.h is working around it for VS 2008. Do we even support VS 2008 any more?

src/node_counters.cc
+
+void InitPerfCounters(Handle<Object> target) {
+ static struct {
+ const char *name;
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

const char*

src/node_counters.cc
+ { NULL }
+ };
+
+ for (int i = 0; tab[i].name != NULL; i++) {
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

i < ARRAY_SIZE(tab) (and drop the { NULL })

src/node_counters.cc
+#define NODE_PROBE(name) #name, name
+
+void InitPerfCounters(Handle<Object> target) {
+ static struct {
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Add a HandleScope.

@sblom

sblom Nov 17, 2012

I'm having trouble figuring out exactly how this works. Like just add the line HandleScope scope; at the top of the method?

@piscisaureus

piscisaureus Nov 17, 2012

Member

If you care, I could really explain how this works another day (ping me on irc).

src/node_counters.cc
+ counter_gc_end_time = counter_gc_start_time;
+
+ v8::V8::AddGCPrologueCallback((GCPrologueCallback)counter_gc_start);
+ v8::V8::AddGCEpilogueCallback((GCEpilogueCallback)counter_gc_done);
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Drop the casts and change the function prototypes, they should return void.

src/node_counters.h
+#endif
+
+#endif
+
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Extraneous newline.

src/node_win32_perfctr_provider.cc
+
+void InitPerfCountersWin32() {
+ ULONG Status;
+ PERF_PROVIDER_CONTEXT ProviderContext;
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Style: s/Status/status/ and s/ProviderContext/provider_context/ (or possibly providerContext). Only classes start with an uppercase character.

Same goes for NodeCounterProvider.

@sblom

sblom Nov 17, 2012

NodeCounterProvider is the name of a code-genned extern (by the perfctr tools)--so I'll leave it, but the others are fixed.

src/node_win32_perfctr_provider.cc
+ perfctr_incrementULongLongValue(NodeCounterProvider,
+ perfctr_instance,
+ NODE_COUNTER_NET_BYTES_SENT,
+ static_cast<ULONGLONG>(bytes));
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Alignment.

src/node_win32_perfctr_provider.cc
+ if (QueryPerformanceCounter(&timegc)) {
+ return timegc.QuadPart;
+ } else {
+ return static_cast<uint64_t>(GetTickCount());
@bnoordhuis

bnoordhuis Nov 16, 2012

Member

Indent errors.

Member

piscisaureus commented Nov 21, 2012

Landed in f657ce6. Thanks @HenryRawas and @sblom.

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