Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

async_hooks: improve resource stack performance #33575

Closed
wants to merge 1 commit into from
Closed

async_hooks: improve resource stack performance #33575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 43 additions & 20 deletions lib/internal/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const {
ObjectPrototypeHasOwnProperty,
ObjectDefineProperty,
Promise,
SafeMap,
Symbol,
} = primordials;

Expand Down Expand Up @@ -38,7 +39,6 @@ const async_wrap = internalBinding('async_wrap');
const {
async_hook_fields,
async_id_fields,
execution_async_resources
} = async_wrap;
// Store the pair executionAsyncId and triggerAsyncId in a AliasedFloat64Array
// in Environment::AsyncHooks::async_ids_stack_ which tracks the resource
Expand All @@ -47,7 +47,9 @@ const {
// each hook's after() callback.
const {
pushAsyncContext: pushAsyncContext_,
popAsyncContext: popAsyncContext_
popAsyncContext: popAsyncContext_,
clearAsyncIdStack,
executionAsyncResource: executionAsyncResource_,
} = async_wrap;
// For performance reasons, only track Promises when a hook is enabled.
const { enablePromiseHook, disablePromiseHook } = async_wrap;
Expand Down Expand Up @@ -84,9 +86,11 @@ const { resource_symbol, owner_symbol } = internalBinding('symbols');
// Each constant tracks how many callbacks there are for any given step of
// async execution. These are tracked so if the user didn't include callbacks
// for a given step, that step can bail out early.
const { kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve,
kCheck, kExecutionAsyncId, kAsyncIdCounter, kTriggerAsyncId,
kDefaultTriggerAsyncId, kStackLength } = async_wrap.constants;
const {
kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve,
kCheck, kExecutionAsyncId, kAsyncIdCounter, kTriggerAsyncId,
kDefaultTriggerAsyncId, kCachedResourceIsValid, kStackLength
} = async_wrap.constants;

const { async_id_symbol,
trigger_async_id_symbol } = internalBinding('symbols');
Expand All @@ -104,12 +108,24 @@ const emitPromiseResolveNative =
emitHookFactory(promise_resolve_symbol, 'emitPromiseResolveNative');

const topLevelResource = {};
// Contains a [stack height] -> [resource] map for things pushed onto the
// async resource stack from JS.
const jsResourceStack = new SafeMap();
// Contains either a single key (null) or nothing. If the key is present,
// this points to the current async resource.
const cachedResourceHolder = new SafeMap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this single key Map be replaced with a variable slot that can be either null or the resource object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that it needs to be some sort of object that can be easily modified from C++ as well (at least until we have weak references in JS without a flag)… I picked a map because clearing it from C++ never throws exceptions, but it could also be a single-entry Array or object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, yes, this could be a single variable slot, but that might become an issue with memory retention in some weird edge cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use a symbol property? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Qard Symbol property on what/in place of what? If you’re talking about the map key, that would work just as well, yes. If you’re talking about replacing the map with an object with a single symbol key, that’s harder to clear from C++ than a map, I’d say?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, we can remove quite a bit of complexity here once we have weak refs without a flag. That would be the actual correct thing to use here. (The internal WeakRef we use elsewhere most likely isn’t fast enough, so we’ll need to wait for V8’s one.)


function executionAsyncResource() {
const index = async_hook_fields[kStackLength] - 1;
if (index === -1) return topLevelResource;
const resource = execution_async_resources[index];
return lookupPublicResource(resource);

if (async_hook_fields[kCachedResourceIsValid])
return cachedResourceHolder.get(null);
const resource = jsResourceStack.get(index) ?? executionAsyncResource_();
const publicResource = lookupPublicResource(resource);
async_hook_fields[kCachedResourceIsValid] = 1;
cachedResourceHolder.set(null, publicResource);
return publicResource;
}

// Used to fatally abort the process if a callback throws.
Expand Down Expand Up @@ -450,16 +466,6 @@ function emitDestroyScript(asyncId) {
}


// Keep in sync with Environment::AsyncHooks::clear_async_id_stack
// in src/env-inl.h.
function clearAsyncIdStack() {
async_id_fields[kExecutionAsyncId] = 0;
async_id_fields[kTriggerAsyncId] = 0;
async_hook_fields[kStackLength] = 0;
execution_async_resources.splice(0, execution_async_resources.length);
}


function hasAsyncIdStack() {
return hasHooks(kStackLength);
}
Expand All @@ -472,15 +478,22 @@ function pushAsyncContext(asyncId, triggerAsyncId, resource) {
return pushAsyncContext_(asyncId, triggerAsyncId, resource);
async_wrap.async_ids_stack[offset * 2] = async_id_fields[kExecutionAsyncId];
async_wrap.async_ids_stack[offset * 2 + 1] = async_id_fields[kTriggerAsyncId];
execution_async_resources[offset] = resource;
async_hook_fields[kStackLength]++;
async_id_fields[kExecutionAsyncId] = asyncId;
async_id_fields[kTriggerAsyncId] = triggerAsyncId;
jsResourceStack.set(offset, resource);
if (async_hook_fields[kCachedResourceIsValid])
cachedResourceHolder.clear();
async_hook_fields[kCachedResourceIsValid] = 0;
}


// This is the equivalent of the native pop_async_ids() call.
function popAsyncContext(asyncId) {
async_hook_fields[kCachedResourceIsValid] = 0;
if (async_hook_fields[kCachedResourceIsValid])
cachedResourceHolder.clear();

const stackLength = async_hook_fields[kStackLength];
if (stackLength === 0) return false;

Expand All @@ -490,9 +503,17 @@ function popAsyncContext(asyncId) {
}

const offset = stackLength - 1;

if (!jsResourceStack.has(offset)) {
// For some reason this popAsyncContext() call removes a resource for a
// corresponding push() call from C++, so let the C++ code handle this
// pop operation since it's on the native the resource stack.
return popAsyncContext_(asyncId);
}

async_id_fields[kExecutionAsyncId] = async_wrap.async_ids_stack[2 * offset];
async_id_fields[kTriggerAsyncId] = async_wrap.async_ids_stack[2 * offset + 1];
execution_async_resources.pop();
jsResourceStack.delete(offset);
async_hook_fields[kStackLength] = offset;
return offset > 0;
}
Expand Down Expand Up @@ -547,5 +568,7 @@ module.exports = {
after: emitAfterNative,
destroy: emitDestroyNative,
promise_resolve: emitPromiseResolveNative
}
},
jsResourceStack,
cachedResourceHolder
};
7 changes: 5 additions & 2 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,11 @@ if (credentials.implementsPosixCredentials) {
// process. They use the same functions as the JS embedder API. These callbacks
// are setup immediately to prevent async_wrap.setupHooks() from being hijacked
// and the cost of doing so is negligible.
const { nativeHooks } = require('internal/async_hooks');
internalBinding('async_wrap').setupHooks(nativeHooks);
const {
nativeHooks, jsResourceStack, cachedResourceHolder
} = require('internal/async_hooks');
internalBinding('async_wrap').setupHooks(
nativeHooks, jsResourceStack, cachedResourceHolder);

const {
setupTaskQueue,
Expand Down
23 changes: 19 additions & 4 deletions src/async_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ using v8::HandleScope;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::Map;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Name;
Expand Down Expand Up @@ -415,6 +416,11 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
SET_HOOK_FN(destroy);
SET_HOOK_FN(promise_resolve);
#undef SET_HOOK_FN

CHECK(args[1]->IsMap());
CHECK(args[2]->IsMap());
env->async_hooks()->set_js_execution_async_resources(args[1].As<Map>());
env->async_hooks()->set_cached_resource_holder(args[2].As<Map>());
}

static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {
Expand Down Expand Up @@ -566,6 +572,16 @@ Local<FunctionTemplate> AsyncWrap::GetConstructorTemplate(Environment* env) {
return tmpl;
}

static void ExecutionAsyncResource(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
args.GetReturnValue().Set(env->async_hooks()->execution_async_resource());
}

static void ClearAsyncIdStack(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->async_hooks()->clear_async_id_stack();
}

void AsyncWrap::Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context,
Expand All @@ -581,6 +597,8 @@ void AsyncWrap::Initialize(Local<Object> target,
env->SetMethod(target, "enablePromiseHook", EnablePromiseHook);
env->SetMethod(target, "disablePromiseHook", DisablePromiseHook);
env->SetMethod(target, "registerDestroyHook", RegisterDestroyHook);
env->SetMethod(target, "executionAsyncResource", ExecutionAsyncResource);
env->SetMethod(target, "clearAsyncIdStack", ClearAsyncIdStack);

PropertyAttribute ReadOnlyDontDelete =
static_cast<PropertyAttribute>(ReadOnly | DontDelete);
Expand Down Expand Up @@ -613,10 +631,6 @@ void AsyncWrap::Initialize(Local<Object> target,
"async_id_fields",
env->async_hooks()->async_id_fields().GetJSArray());

FORCE_SET_TARGET_FIELD(target,
"execution_async_resources",
env->async_hooks()->execution_async_resources());

target->Set(context,
env->async_ids_stack_string(),
env->async_hooks()->async_ids_stack().GetJSArray()).Check();
Expand All @@ -637,6 +651,7 @@ void AsyncWrap::Initialize(Local<Object> target,
SET_HOOKS_CONSTANT(kTriggerAsyncId);
SET_HOOKS_CONSTANT(kAsyncIdCounter);
SET_HOOKS_CONSTANT(kDefaultTriggerAsyncId);
SET_HOOKS_CONSTANT(kCachedResourceIsValid);
SET_HOOKS_CONSTANT(kStackLength);
#undef SET_HOOKS_CONSTANT
FORCE_SET_TARGET_FIELD(target, "constants", constants);
Expand Down
78 changes: 67 additions & 11 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,32 @@ inline AliasedFloat64Array& AsyncHooks::async_ids_stack() {
return async_ids_stack_;
}

inline v8::Local<v8::Array> AsyncHooks::execution_async_resources() {
return PersistentToLocal::Strong(execution_async_resources_);
v8::Local<v8::Value> AsyncHooks::execution_async_resource() {
if (fields_[kStackLength] == 0) return {};
uint32_t offset = fields_[kStackLength] - 1;
if (LIKELY(offset < native_execution_async_resources_.size()))
return PersistentToLocal::Strong(native_execution_async_resources_[offset]);

// This async resource was stored in the JS async resource map.
v8::Local<v8::Map> js_map =
PersistentToLocal::Strong(js_execution_async_resources_);
v8::Local<v8::Integer> key =
v8::Integer::NewFromUnsigned(env()->isolate(), offset);
v8::Local<v8::Value> ret;
if (UNLIKELY(js_map.IsEmpty() ||
!js_map->Get(env()->context(), key).ToLocal(&ret) ||
ret->IsUndefined())) {
return {};
}
return ret;
}

void AsyncHooks::set_js_execution_async_resources(v8::Local<v8::Map> value) {
js_execution_async_resources_.Reset(env()->isolate(), value);
}

void AsyncHooks::set_cached_resource_holder(v8::Local<v8::Map> value) {
cached_resource_holder_.Reset(env()->isolate(), value);
}

inline v8::Local<v8::String> AsyncHooks::provider_string(int idx) {
Expand Down Expand Up @@ -151,12 +175,23 @@ inline void AsyncHooks::push_async_context(double async_id,
async_id_fields_[kExecutionAsyncId] = async_id;
async_id_fields_[kTriggerAsyncId] = trigger_async_id;

auto resources = execution_async_resources();
USE(resources->Set(env()->context(), offset, resource));
#ifdef DEBUG
for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++)
CHECK(native_execution_async_resources_[i].IsEmpty());
#endif
native_execution_async_resources_.resize(offset + 1);
native_execution_async_resources_[offset].Reset(env()->isolate(), resource);
if (fields_[kCachedResourceIsValid])
PersistentToLocal::Strong(cached_resource_holder_)->Clear();
fields_[kCachedResourceIsValid] = 0;
}

// Remember to keep this code aligned with popAsyncContext() in JS.
inline bool AsyncHooks::pop_async_context(double async_id) {
if (fields_[kCachedResourceIsValid])
PersistentToLocal::Strong(cached_resource_holder_)->Clear();
fields_[kCachedResourceIsValid] = 0;

// In case of an exception then this may have already been reset, if the
// stack was multiple MakeCallback()'s deep.
if (fields_[kStackLength] == 0) return false;
Expand Down Expand Up @@ -185,21 +220,42 @@ inline bool AsyncHooks::pop_async_context(double async_id) {
async_id_fields_[kTriggerAsyncId] = async_ids_stack_[2 * offset + 1];
fields_[kStackLength] = offset;

auto resources = execution_async_resources();
USE(resources->Delete(env()->context(), offset));
if (LIKELY(offset < native_execution_async_resources_.size() &&
!native_execution_async_resources_[offset].IsEmpty())) {
#ifdef DEBUG
for (uint32_t i = offset + 1;
i < native_execution_async_resources_.size();
i++) {
CHECK(native_execution_async_resources_[i].IsEmpty());
}
#endif
native_execution_async_resources_[offset].Reset();
native_execution_async_resources_.resize(offset);
if (native_execution_async_resources_.size() <
native_execution_async_resources_.capacity() / 2) {
native_execution_async_resources_.shrink_to_fit();
}
} else {
USE(PersistentToLocal::Strong(js_execution_async_resources_)->Delete(
env()->context(),
v8::Integer::NewFromUnsigned(env()->isolate(), offset)));
}

return fields_[kStackLength] > 0;
}

// Keep in sync with clearAsyncIdStack in lib/internal/async_hooks.js.
inline void AsyncHooks::clear_async_id_stack() {
auto isolate = env()->isolate();
v8::HandleScope handle_scope(isolate);
execution_async_resources_.Reset(isolate, v8::Array::New(isolate));
void AsyncHooks::clear_async_id_stack() {
if (!js_execution_async_resources_.IsEmpty()) {
PersistentToLocal::Strong(cached_resource_holder_)->Clear();
PersistentToLocal::Strong(js_execution_async_resources_)->Clear();
}
native_execution_async_resources_.clear();
native_execution_async_resources_.shrink_to_fit();

async_id_fields_[kExecutionAsyncId] = 0;
async_id_fields_[kTriggerAsyncId] = 0;
fields_[kStackLength] = 0;
fields_[kCachedResourceIsValid] = 0;
}

// The DefaultTriggerAsyncIdScope(AsyncWrap*) constructor is defined in
Expand Down
9 changes: 7 additions & 2 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,7 @@ class AsyncHooks : public MemoryRetainer {
kTotals,
kCheck,
kStackLength,
kCachedResourceIsValid,
kFieldsCount,
};

Expand All @@ -653,7 +654,7 @@ class AsyncHooks : public MemoryRetainer {
inline AliasedUint32Array& fields();
inline AliasedFloat64Array& async_id_fields();
inline AliasedFloat64Array& async_ids_stack();
inline v8::Local<v8::Array> execution_async_resources();
inline v8::Local<v8::Value> execution_async_resource();

inline v8::Local<v8::String> provider_string(int idx);

Expand All @@ -664,6 +665,8 @@ class AsyncHooks : public MemoryRetainer {
v8::Local<v8::Value> execution_async_resource_);
inline bool pop_async_context(double async_id);
inline void clear_async_id_stack(); // Used in fatal exceptions.
inline void set_js_execution_async_resources(v8::Local<v8::Map> value);
inline void set_cached_resource_holder(v8::Local<v8::Map> value);

AsyncHooks(const AsyncHooks&) = delete;
AsyncHooks& operator=(const AsyncHooks&) = delete;
Expand Down Expand Up @@ -706,7 +709,9 @@ class AsyncHooks : public MemoryRetainer {

void grow_async_ids_stack();

v8::Global<v8::Array> execution_async_resources_;
std::vector<v8::Global<v8::Value>> native_execution_async_resources_;
v8::Global<v8::Map> js_execution_async_resources_;
v8::Global<v8::Map> cached_resource_holder_;
};

class ImmediateInfo : public MemoryRetainer {
Expand Down