os: improve cpus() performance #11564

mscdex · 2017-02-26T04:57:41Z

Results with included benchmark:

                    improvement confidence      p.value
 os/cpus.js n=30000     12.79 %        *** 1.170309e-36

CI: https://ci.nodejs.org/job/node-test-pull-request/6591/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

os

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2017-02-26T11:24:20Z

src/node_os.cc

+    fields[4] = ci->cpu_times.idle;
+    fields[5] = ci->cpu_times.irq;
+    Local<Value> argv[] = {
+      OneByteString(env->isolate(), ci->model)


Another possible enhancement: cache the result and check on the next iteration:

Local<String> model_string; int model_index; // Then inside the loop... if (model_string.IsEmpty() || 0 != strcmp(ci->model, cpu_infos[model_index).model) { model_string = OneByteString(env->isolate(), ci->model); model_index = i; } Local<Value> argv[] = { model_string };

I just tried that in addition to the changes I just made and did not see any difference.

mscdex · 2017-02-26T11:44:57Z

I've now tweaked the code to squeeze out a bit more speed.

CI: https://ci.nodejs.org/job/node-test-pull-request/6592/

JacksonTian · 2017-02-27T01:56:02Z

LGTM

TimothyGu · 2017-02-27T02:07:42Z

src/node_util.cc

+    env->context(),
+    OneByteString(env->isolate(), "pushValToArrayMax"),
+    Integer::NewFromUnsigned(env->isolate(), NODE_PUSH_VAL_TO_ARRAY_MAX),
+    v8::ReadOnly).FromJust();


I assume the old PropertyAttribute-based API is used rather than the ES5 PropertyDescriptor-based one for ease of backporting?

I just copied from the READONLY_PROPERTY macro in src/node.cc.

You could leave out the ReadOnly flag entirely since it's only a bindings object property.

bnoordhuis · 2017-02-27T05:40:03Z

src/node_util.cc

+    env->context(),
+    OneByteString(env->isolate(), "pushValToArrayMax"),
+    Integer::NewFromUnsigned(env->isolate(), NODE_PUSH_VAL_TO_ARRAY_MAX),
+    v8::ReadOnly).FromJust();


You could leave out the ReadOnly flag entirely since it's only a bindings object property.

gibfahn · 2017-02-27T09:57:11Z

@mscdex looks like every test failed on AIX (there were also SmartOS failures, but I'm not sure what caused them).

not ok 1 parallel/test-arm-math-exp-regress-1376
  ---
  duration_ms: 0.209
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 11)
  ...

I suspect this was caused by the os.cpus() call in test/common.js#L37, but unfortunately there isn't enough info in the TAP results to tell.

mscdex · 2017-02-28T03:25:16Z

Alright, after duplicating the issue on a smartos16-64 CI node (base-64 16.2.0), the crash appears to be happening in V8, not in node's code. I did a thread apply all bt in gdb, but all threads except the main thread were doing the same thing ("sleeping"). Here's the backtrace for the main thread:

#0  0xffffbf7fff22176a in _lwp_kill () from /lib/64/libc.so.1
#1  0xffffbf7fff21806f in thr_kill () from /lib/64/libc.so.1
#2  0xffffbf7fff1b5860 in raise () from /lib/64/libc.so.1
#3  0xffffbf7ffec27ec4 in umem_do_abort () from /lib/64/libumem.so.1
#4  0xffffbf7ffec28022 in umem_panic () from /lib/64/libumem.so.1
#5  0xffffbf7ffec2814a in __umem_assert_failed () from /lib/64/libumem.so.1
#6  0xffffbf7ffec2cdf3 in umem_slab_alloc () from /lib/64/libumem.so.1
#7  0xffffbf7ffec2def3 in umem_cache_alloc () from /lib/64/libumem.so.1
#8  0xffffbf7ffec2e2a4 in umem_alloc () from /lib/64/libumem.so.1
#9  0xffffbf7ffec2aa1f in umem_malloc () from /lib/64/libumem.so.1
#10 0x00000000022abf8c in v8::base::DefaultAllocationPolicy::New(unsigned long) ()
#11 0x00000000022cfb5d in v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::Initialize(unsigned int, v8::base::DefaultAllocationPolicy) ()
#12 0x00000000028acf43 in v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::Resize(v8::base::DefaultAllocationPolicy) ()
#13 0x00000000028abda5 in v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::FillEmptyEntry(v8::base::TemplateHashMapEntry<void*, void*>*, void* const&, void* const&, unsigned int, v8::base::DefaultAllocationPolicy) ()
#14 0x00000000028aa7ef in v8::base::TemplateHashMapEntry<void*, void*>* v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::LookupOrInsert<v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::LookupOrInsert(void* const&, unsigned int, v8::base::DefaultAllocationPolicy)::{lambda()#1}>(void* const&, unsigned int, v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::LookupOrInsert(void* const&, unsigned int, v8::base::DefaultAllocationPolicy)::{lambda()#1} const&, v8::base::DefaultAllocationPolicy) ()
#15 0x00000000028a9452 in v8::base::TemplateHashMapImpl<void*, void*, v8::base::HashEqualityThenKeyMatcher<void*, bool (*)(void*, void*)>, v8::base::DefaultAllocationPolicy>::LookupOrInsert(void* const&, unsigned int, v8::base::DefaultAllocationPolicy) ()
#16 0x0000000002b074e2 in v8::internal::AstValueFactory::GetString (this=0x3acac10, hash=3954651106, is_one_byte=true, literal_bytes=...) at ../deps/v8/src/ast/ast-value-factory.cc:381
#17 0x0000000002b06b1b in v8::internal::AstValueFactory::GetOneByteStringInternal (this=0x3acac10, literal=...) at ../deps/v8/src/ast/ast-value-factory.cc:227
#18 0x00000000022efb2e in v8::internal::AstValueFactory::GetOneByteString(v8::internal::Vector<unsigned char const>) ()
#19 0x0000000002889b10 in v8::internal::Scanner::CurrentSymbol (this=0xffffbf7fffdfd9e0, ast_value_factory=0x3acac10) at ../deps/v8/src/parsing/scanner.cc:1577
#20 0x0000000002839037 in v8::internal::Parser::GetSymbol() const ()
#21 0x00000000028455c9 in v8::internal::ParserBase<v8::internal::Parser>::ParseAndClassifyIdentifier(bool*) ()
#22 0x0000000002846978 in v8::internal::ParserBase<v8::internal::Parser>::ParsePrimaryExpression(bool*, bool*) ()
#23 0x0000000002844d5f in v8::internal::ParserBase<v8::internal::Parser>::ParsePrimaryExpression(bool*) ()
#24 0x000000000283fa0d in v8::internal::ParserBase<v8::internal::Parser>::ParseFormalParameter(v8::internal::ParserFormalParameters*, bool*) ()
#25 0x000000000283f860 in v8::internal::ParserBase<v8::internal::Parser>::ParseFormalParameterList(v8::internal::ParserFormalParameters*, bool*) ()
#26 0x00000000028293ac in v8::internal::Parser::ParseFunction (this=0xffffbf7fffdfd960, function_name=0x3ac4560, pos=2396, kind=v8::internal::kNormalFunction, 
    function_type=v8::internal::FunctionLiteral::kAnonymousExpression, function_scope=0x3ac48d8, num_parameters=0xffffbf7fffdfd4ec, function_length=0xffffbf7fffdfd4e8, 
    has_duplicate_parameters=0xffffbf7fffdfd4e7, materialized_literal_count=0xffffbf7fffdfd4f4, expected_property_count=0xffffbf7fffdfd4f0, ok=0xffffbf7fffdfd70f) at ../deps/v8/src/parsing/parser.cc:3127
#27 0x0000000002827251 in v8::internal::Parser::ParseFunctionLiteral (this=0xffffbf7fffdfd960, function_name=0x3ac4560, function_name_location=..., 
    function_name_validity=v8::internal::kSkipFunctionNameCheck, kind=v8::internal::kNormalFunction, function_token_pos=-1, function_type=v8::internal::FunctionLiteral::kAnonymousExpression, 
    language_mode=v8::internal::STRICT, ok=0xffffbf7fffdfd70f) at ../deps/v8/src/parsing/parser.cc:2683
#28 0x0000000002820e2d in v8::internal::Parser::DoParseFunction (this=0xffffbf7fffdfd960, info=0xffffbf7fffdfdd70, raw_name=0x3ac4560, source=0x3ab9050) at ../deps/v8/src/parsing/parser.cc:1001
#29 0x000000000282028f in v8::internal::Parser::ParseFunction (this=0xffffbf7fffdfd960, isolate=0x3a3b010, info=0xffffbf7fffdfdd70) at ../deps/v8/src/parsing/parser.cc:854
#30 0x000000000282b905 in v8::internal::Parser::Parse (this=0xffffbf7fffdfd960, info=0xffffbf7fffdfdd70) at ../deps/v8/src/parsing/parser.cc:3806
#31 0x000000000282b7bf in v8::internal::Parser::ParseStatic (info=0xffffbf7fffdfdd70) at ../deps/v8/src/parsing/parser.cc:3787
#32 0x00000000024a3c47 in v8::internal::(anonymous namespace)::GetUnoptimizedCode (info=0xffffbf7fffdfde10) at ../deps/v8/src/compiler.cc:456
#33 0x00000000024a6145 in v8::internal::(anonymous namespace)::GetLazyCode (function=...) at ../deps/v8/src/compiler.cc:929
#34 0x00000000024a6d10 in v8::internal::Compiler::Compile (function=..., flag=v8::internal::Compiler::KEEP_EXCEPTION) at ../deps/v8/src/compiler.cc:1052
#35 0x0000000002d5c612 in v8::internal::__RT_impl_Runtime_CompileLazy (args=..., isolate=0x3a3b010) at ../deps/v8/src/runtime/runtime-compiler.cc:38
#36 0x0000000002d5c46b in v8::internal::Runtime_CompileLazy (args_length=1, args_object=0xffffbf7fffdfe160, isolate=0x3a3b010) at ../deps/v8/src/runtime/runtime-compiler.cc:23

The example test I used to trigger this was test/parallel/test-async-wrap-check-providers.js.

/cc @nodejs/v8 ?

bnoordhuis · 2017-02-28T10:14:06Z

Does it also crash with node --nolazy test/parallel/test-async-wrap-check-providers.js?

cc @nodejs/platform-smartos because it happens in libumem.

mscdex · 2017-02-28T13:41:19Z

@bnoordhuis With that flag it still crashes. This time the backtrace is:

Thread 2 received signal SIGABRT, Aborted.
[Switching to Thread 1 (LWP 1)]
0xffffbf7fff22176a in _lwp_kill () from /lib/64/libc.so.1
(gdb) bt
#0  0xffffbf7fff22176a in _lwp_kill () from /lib/64/libc.so.1
#1  0xffffbf7fff21806f in thr_kill () from /lib/64/libc.so.1
#2  0xffffbf7fff1b5860 in raise () from /lib/64/libc.so.1
#3  0xffffbf7ffec27ec4 in umem_do_abort () from /lib/64/libumem.so.1
#4  0xffffbf7ffec28022 in umem_panic () from /lib/64/libumem.so.1
#5  0xffffbf7ffec2814a in __umem_assert_failed () from /lib/64/libumem.so.1
#6  0xffffbf7ffec2cdf3 in umem_slab_alloc () from /lib/64/libumem.so.1
#7  0xffffbf7ffec2def3 in umem_cache_alloc () from /lib/64/libumem.so.1
#8  0xffffbf7ffec2e2a4 in umem_alloc () from /lib/64/libumem.so.1
#9  0xffffbf7ffec2aa1f in umem_malloc () from /lib/64/libumem.so.1
#10 0x00000000025a1857 in v8::internal::FrameDescription::operator new(unsigned long, unsigned int) ()
#11 0x00000000025934fd in v8::internal::Deoptimizer::Deoptimizer (this=0x3d2c2d0, isolate=0x3a3b010, function=0x0, type=v8::internal::Deoptimizer::LAZY, bailout_id=0, 
    from=0x3e52869406d4 "H\203\354\b\377t$\bARH\211l$\030H\215l$\030I\272", fp_to_sp_delta=8) at ../deps/v8/src/deoptimizer.cc:503
#12 0x0000000002591b75 in v8::internal::Deoptimizer::New (function=0x0, type=v8::internal::Deoptimizer::LAZY, bailout_id=0, from=0x3e52869406d4 "H\203\354\b\377t$\bARH\211l$\030H\215l$\030I\272", 
    fp_to_sp_delta=8, isolate=0x3a3b010) at ../deps/v8/src/deoptimizer.cc:76

mscdex · 2017-03-02T10:49:19Z

False alarm, it was just a bug in the code I added. It only appeared on the SmartOS and AIX CI machines because they had a large enough CPU count to trigger the bug.

New CI: https://ci.nodejs.org/job/node-test-pull-request/6653/

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2017-03-02T10:52:32Z

src/node_os.cc

+    if (model_idx >= NODE_PUSH_VAL_TO_ARRAY_MAX) {
+      addfn->Call(env->context(), cpus, model_idx, model_argv).ToLocalChecked();
+      model_idx = 0;
+      f = 0;


Call this field_idx maybe?

mscdex · 2017-03-02T11:25:49Z

CI is green except for unrelated test failures. Hooray!

gibfahn · 2017-03-02T12:53:37Z

Rerun of CI was even more green!

CI: https://ci.nodejs.org/job/node-test-commit/8204/

PR-URL: nodejs#11564 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jackson Tian <shyvo1987@gmail.com>

PR-URL: #11564 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jackson Tian <shyvo1987@gmail.com>

MylesBorins · 2017-04-17T21:34:51Z

@mscdex thoughts on backport?

mscdex added os Issues and PRs related to the os subsystem. performance Issues and PRs related to the performance of Node.js. c++ Issues and PRs that require attention from people who are familiar with C++. wip Issues and PRs that are still a work in progress. labels Feb 26, 2017

bnoordhuis approved these changes Feb 26, 2017

View reviewed changes

mscdex force-pushed the os-cpus-perf branch from 07a203e to e5d4698 Compare February 26, 2017 11:41

mscdex removed the wip Issues and PRs that are still a work in progress. label Feb 26, 2017

jasnell approved these changes Feb 27, 2017

View reviewed changes

TimothyGu reviewed Feb 27, 2017

View reviewed changes

bnoordhuis approved these changes Feb 27, 2017

View reviewed changes

mscdex force-pushed the os-cpus-perf branch from e5d4698 to 6758c8d Compare March 2, 2017 10:46

bnoordhuis approved these changes Mar 2, 2017

View reviewed changes

mscdex force-pushed the os-cpus-perf branch from 6758c8d to 7eb4b09 Compare March 2, 2017 10:58

os: improve cpus() performance

4e05952

PR-URL: nodejs#11564 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jackson Tian <shyvo1987@gmail.com>

mscdex force-pushed the os-cpus-perf branch from 7eb4b09 to 4e05952 Compare March 2, 2017 20:33

mscdex merged commit 4e05952 into nodejs:master Mar 2, 2017

mscdex deleted the os-cpus-perf branch March 2, 2017 20:37

addaleax pushed a commit that referenced this pull request Mar 5, 2017

os: improve cpus() performance

e2133f3

PR-URL: #11564 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jackson Tian <shyvo1987@gmail.com>

TimothyGu mentioned this pull request Mar 6, 2017

Illegal invocation error using ES6 Proxy and node.js #11629

Closed

evanlucas mentioned this pull request Mar 8, 2017

v7.7.2 proposal #11745

Merged

MylesBorins added the lts-watch-v6.x label Apr 17, 2017

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 15, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: improve cpus() performance #11564

os: improve cpus() performance #11564

mscdex commented Feb 26, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Feb 26, 2017

mscdex Feb 26, 2017

mscdex commented Feb 26, 2017 •

edited

Loading

JacksonTian commented Feb 27, 2017

TimothyGu Feb 27, 2017

mscdex Feb 27, 2017

bnoordhuis Feb 27, 2017

bnoordhuis Feb 27, 2017

gibfahn commented Feb 27, 2017 •

edited

Loading

mscdex commented Feb 28, 2017 •

edited

Loading

bnoordhuis commented Feb 28, 2017

mscdex commented Feb 28, 2017

mscdex commented Mar 2, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Mar 2, 2017

mscdex Mar 2, 2017

mscdex commented Mar 2, 2017

gibfahn commented Mar 2, 2017

MylesBorins commented Apr 17, 2017

os: improve cpus() performance #11564

os: improve cpus() performance #11564

Conversation

mscdex commented Feb 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Feb 26, 2017 • edited Loading

JacksonTian commented Feb 27, 2017

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

gibfahn commented Feb 27, 2017 • edited Loading

mscdex commented Feb 28, 2017 • edited Loading

bnoordhuis commented Feb 28, 2017

mscdex commented Feb 28, 2017

mscdex commented Mar 2, 2017 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 2, 2017

gibfahn commented Mar 2, 2017

MylesBorins commented Apr 17, 2017

mscdex commented Feb 26, 2017 •

edited

Loading

mscdex commented Feb 26, 2017 •

edited

Loading

gibfahn commented Feb 27, 2017 •

edited

Loading

mscdex commented Feb 28, 2017 •

edited

Loading

mscdex commented Mar 2, 2017 •

edited

Loading