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

n-api: cache Symbol.hasInstance #12246

Closed
wants to merge 1 commit into from
Closed

n-api: cache Symbol.hasInstance #12246

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
61 changes: 41 additions & 20 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,16 @@ void napi_clear_last_error(napi_env env);

class napi_env__ {
public:
explicit napi_env__(v8::Isolate* _isolate): isolate(_isolate), last_error() {}
explicit napi_env__(v8::Isolate* _isolate): isolate(_isolate),
has_instance_available(true), last_error() {}
~napi_env__() {
last_exception.Reset();
has_instance.Reset();
}
v8::Isolate* isolate;
v8::Persistent<v8::Value> last_exception;
v8::Persistent<v8::Value> has_instance;
Copy link
Member

Choose a reason for hiding this comment

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

do we need a copy in every env ?

Copy link
Member

Choose a reason for hiding this comment

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

ie could we share one copy across all envs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That brings back the question of what prompts the creation of a new env? Currently it is the loading of a module. But should it be a new isolate?

Also, how likely is this to be a large waste of memory? I mean, are we going to have tons of native modules loaded all at once?

I guess when there really start to appear multiple isolates (or whatever keys meaning "this JS world and not the other" because "this v8::Local<v8::Value> is only valid in this world") then we can re-examine our choice of one env per module, because the problem will have been solved upstream and we need only follow suit. We might then introduce a std::map keyed to whatever the engine then provides, or we may have a better mechanism for tacking data onto such a key and we'll once again cast.

... and we can do all this without breaking any ABI, since it's purely within our implementation.

Copy link
Member

Choose a reason for hiding this comment

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

I just wondered if the exact same code, except that having has_instance be static would work. The cache would be created the first time an env was created. The only concern was whether this could happen in parallel, but given Node.js generally single thread nature that may not be an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson because eventually the env will be keyed to some kind of context, and we are preparing ourselves for that. Otherwise last_exception could also be static - and it used to be, and the env was precisely equal to the isolate. The problem is we can't currently tack onto the isolate because there is no reliable mechanism for that, so we're instantiating an environment per module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, yes, it would work perfectly, and it's only our desire to be proactive that prompted us to implement this solution.

Copy link
Member

Choose a reason for hiding this comment

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

ok, got it.

bool has_instance_available;
napi_extended_error_info last_error;
};

Expand Down Expand Up @@ -2149,28 +2153,43 @@ napi_status napi_instanceof(napi_env env,
return napi_set_last_error(env, napi_function_expected);
}

napi_value value, js_result;
napi_status status;
napi_valuetype value_type;
if (env->has_instance_available) {
napi_value value, js_result, has_instance = nullptr;
napi_status status;
napi_valuetype value_type;

// Get "Symbol" from the global object
status = napi_get_global(env, &value);
if (status != napi_ok) return status;
status = napi_get_named_property(env, value, "Symbol", &value);
if (status != napi_ok) return status;
status = napi_typeof(env, value, &value_type);
if (status != napi_ok) return status;
// Get "Symbol" from the global object
if (env->has_instance.IsEmpty()) {
status = napi_get_global(env, &value);
if (status != napi_ok) return status;
status = napi_get_named_property(env, value, "Symbol", &value);
if (status != napi_ok) return status;
status = napi_typeof(env, value, &value_type);
if (status != napi_ok) return status;

// Get "hasInstance" from Symbol
if (value_type == napi_function) {
status = napi_get_named_property(env, value, "hasInstance", &value);
if (status != napi_ok) return status;
status = napi_typeof(env, value, &value_type);
if (status != napi_ok) return status;
// Get "hasInstance" from Symbol
if (value_type == napi_function) {
status = napi_get_named_property(env, value, "hasInstance", &value);
if (status != napi_ok) return status;
status = napi_typeof(env, value, &value_type);
if (status != napi_ok) return status;

// Store Symbol.hasInstance in a global persistent reference
if (value_type == napi_symbol) {
env->has_instance.Reset(env->isolate,
v8impl::V8LocalValueFromJsValue(value));
if (status != napi_ok) return status;
has_instance = value;
}
}
} else {
has_instance = v8impl::JsValueFromV8LocalValue(
v8::Local<v8::Value>::New(env->isolate, env->has_instance));
if (status != napi_ok) return status;
}

// Retrieve the function at the Symbol(hasInstance) key of the constructor
if (value_type == napi_symbol) {
status = napi_get_property(env, constructor, value, &value);
if (has_instance) {
status = napi_get_property(env, constructor, has_instance, &value);
if (status != napi_ok) return status;
status = napi_typeof(env, value, &value_type);
if (status != napi_ok) return status;
Expand All @@ -2184,6 +2203,8 @@ napi_status napi_instanceof(napi_env env,
return napi_get_value_bool(env, js_result, result);
}
}

env->has_instance_available = false;
}

// If running constructor[Symbol.hasInstance](object) did not work, we perform
Expand Down