src: use an array for faster binding data lookup #46620
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
joyeecheung
force-pushed
the
type-int
branch
from
e25375f
to
0a0df16
Compare
Do you know where this hashing happens in the stack/can share how you profiled this? The whole point of |
I used perf to profile a new binding that calls |
|
@joyeecheung Here’s my local assembly for in the fold000000000098fba0 <node::http2::PackSettings(v8::FunctionCallbackInfo<v8::Value> const&)>:
98fba0: f3 0f 1e fa endbr64
98fba4: 55 push %rbp
98fba5: 48 89 e5 mov %rsp,%rbp
98fba8: 53 push %rbx
98fba9: 48 89 fb mov %rdi,%rbx
98fbac: 48 83 ec 08 sub $0x8,%rsp
98fbb0: 48 8b 07 mov (%rdi),%rax
98fbb3: 48 8b 78 08 mov 0x8(%rax),%rdi
98fbb7: e8 34 5f 1e 00 call b75af0 <v8::Isolate::GetCurrentContext()>
98fbbc: 31 d2 xor %edx,%edx
98fbbe: 48 b9 97 8c 9a 0f 31 movabs $0x310f9a8c97,%rcx
98fbc5: 00 00 00
98fbc8: 48 8b 00 mov (%rax),%rax
98fbcb: 48 8b 40 2f mov 0x2f(%rax),%rax
98fbcf: 48 8b b8 27 01 00 00 mov 0x127(%rax),%rdi
98fbd6: 48 89 c8 mov %rcx,%rax
98fbd9: 48 f7 77 08 divq 0x8(%rdi)
98fbdd: 48 89 d6 mov %rdx,%rsi
98fbe0: 48 8d 15 d9 23 39 04 lea 0x43923d9(%rip),%rdx # 4d21fc0 <node::http2::Http2State::type_name>
98fbe7: e8 24 d1 f8 ff call 91cd10 <std::_Hashtable<node::FastStringKey, std::pair<node::FastStringKey const, node::BaseObjectPtrImpl<node::BaseObject, false> >, std::allocator<std::pair<node::FastStringKey const, node::BaseObjectPtrImpl<node::BaseObject, false> > >, std::__detail::_Select1st, std::equal_to<node::FastStringKey>, node::FastStringKey::Hash, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_find_before_node(unsigned long, node::FastStringKey const&, unsigned long) const>
98fbec: 48 89 c7 mov %rax,%rdi
98fbef: 48 85 c0 test %rax,%rax
98fbf2: 74 0c je 98fc00 <node::http2::PackSettings(v8::FunctionCallbackInfo<v8::Value> const&)+0x60>
98fbf4: 48 8b 38 mov (%rax),%rdi
98fbf7: 48 85 ff test %rdi,%rdi
98fbfa: 74 04 je 98fc00 <node::http2::PackSettings(v8::FunctionCallbackInfo<v8::Value> const&)+0x60>
98fbfc: 48 8b 7f 20 mov 0x20(%rdi),%rdi
98fc00: 48 8b 1b mov (%rbx),%rbx
98fc03: e8 78 94 fe ff call 979080 <node::http2::Http2Settings::Pack(node::http2::Http2State*)>
98fc08: 48 85 c0 test %rax,%rax
98fc0b: 74 13 je 98fc20 <node::http2::PackSettings(v8::FunctionCallbackInfo<v8::Value> const&)+0x80>
98fc0d: 48 8b 00 mov (%rax),%rax
98fc10: 48 89 43 18 mov %rax,0x18(%rbx)
98fc14: 48 8b 5d f8 mov -0x8(%rbp),%rbx
98fc18: c9 leave
98fc19: c3 ret
98fc1a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
98fc20: 48 8b 43 10 mov 0x10(%rbx),%rax
98fc24: 48 89 43 18 mov %rax,0x18(%rbx)
98fc28: 48 8b 5d f8 mov -0x8(%rbp),%rbx
98fc2c: c9 leave
98fc2d: c3 ret
98fc2e: 66 90 xchg %ax,%axThe hash (0x310f9a8c97) is hardcoded here (as it should be – it’s |
|
This is the branch I used to find out the profile, by the way, revert the last two commits to go back to type_name, and use See code'use strict';
const encoder = new TextEncoder('utf-8');
const arr = new Uint8Array(1000);
const str = new Date().toISOString();
let result = [];
for (let i = 0; i < 1000000; ++i) {
result = encoder.encodeInto(str, arr);
}
console.log(result);And I'd get: See code(and some regressions, even without the AliasedBuffer change) |
|
My guess is hard-coding is not the same as the hashing still needs to be done (e.g. it still needs to do what it does to handle collisions? so extra overhead). I didn't check out the STL implementation but now come to think of it, why do we even need a map when the keys are all just..sequential numbers (starting from 0, even)? |
|
Updated the patch to just use an std::array. Also I think I found why the original hash was slow - there are collisions (notice how in the profile the actual function where time is spent is See logsIt might be possible to optimize the hash function of FastStringKey, but in the case of binding data, that's an overkill for problem as we could really just use the enums as keys and do not need to worry about collisions at all. And also because we could just use the enums as keys, we could just use an array, and that's as fast as it gets. |
joyeecheung
force-pushed
the
type-int
branch
from
4aae96e
to
c841b77
Compare
joyeecheung
changed the title
I suspect that wouldn’t make much of a difference here – the problem isn’t that the hashes collide directly, it’s that the number of buckets is on the lower end and therefore collisions are to be expected regardless of the exact hash function.
The goal of that was just to make the individual implementations more self-contained/avoid the need for global lists of all possible objects of that type. |
Yeah although in the case of binding data, there is already a global list for the bindings, so another global list for the wrapper of the binding object probably isn't that much of a big deal anyway..eventually it might be nicer to just have C++ wrappers for all the bindings (and merge the lists), then we can move more things off the Environment object and make the global states more self-contained, like the TODO I added in #46579 about moving immediate info and timeout info to the binding, which also helps the per-realm/context built-in effort (#46558) I assume |
github-actions
bot
removed
the
request-ci
Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup.
|
Rebased after #46556 |
joyeecheung
force-pushed
the
type-int
branch
from
8a46ff7
to
4becc8f
Compare
Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup. PR-URL: #46620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
|
Landed in f32aa19 |
juanarbol
added
the
backport-requested-v18.x
|
Would you mind backporting this to v18.x |
Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup. PR-URL: #46620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup. PR-URL: #46620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup. PR-URL: nodejs/node#46620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup. PR-URL: nodejs/node#46620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
src: use an array for faster binding data lookup
Locally the hashing of the binding names sometimes has significant
presence in the profile of bindings, because there can be collisions,
which makes the cost of adding a new binding data non-trivial,
but it's wasteful to spend time on hashing them or dealing with
collisions at all, since we can just use the EmbedderObjectType
enum as the key, as the string names are not actually used beyond
debugging purposes and can be easily matched with a macro.
And since we can just use the enum as the key, we do not even
need the map and can just use an array with the enum as indices
for the lookup.
Background: I was trying to move the encoding-related bindings to a new binding with its own BindingData and noticed ~15% regression in some TextEncoder benchmarks, and the hashing showed up in the profile. With this patch the regression goes away and the hashing no longer has any presence in the profile.