os: improve cpus() performance

[mscdex]

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]

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 };

[mscdex]

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

[mscdex]

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]

LGTM

[TimothyGu]

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

[mscdex]

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

[bnoordhuis]

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

[bnoordhuis]

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

[gibfahn]

@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]

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]

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]

@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]

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]

Call this field_idx maybe?

[mscdex]

Changed.

[mscdex]

CI is green except for unrelated test failures. Hooray!

[gibfahn]

Rerun of CI was even more green!

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

[MylesBorins]

@mscdex thoughts on backport?