Observation: Unnecessarily calling napi_set_last_error()

[gabrielschulhof]

If we want to avoid function call overhead maybe we should put a macro around napi_set_last_error() which avoids calling it if static_last_error.error_code is already set to the code which it is instructed to set. I'm not sure if it's worth the overhead savings.

[gabrielschulhof]

I realize that once we have engine-specific codes, two different napi_generic_failures may not be equal, but even then we need only elaborate on the if-condition guarding the function call.

[gabrielschulhof]

#define MAYBE_SET_LAST_ERROR(status) \
  ((static_last_error.error_code == status) ? status : \
    napi_set_last_error(status))

[jasongin]

Wouldn't the call to napi_set_last_error() be inlined? (Note it's not a public API.) We could throw an inline tag on it to give the compiler a suggestion, but I don't know if that would actually make a difference. So, I'm not sure the macro suggested here would make a significant difference.

[gabrielschulhof]

Well, if we can be fairly certain that there won't be a function call, then we can certainly leave things as they are.

[mhdawson]

My preference is to have it be inlined versus making and additional check. Since its internal to the implementation we can always change our mind if we find that it stands out as a problem in future performance measurements.

[gabrielschulhof]

Once #198 is fixed, napi_set_last_error() will receive a napi_env from which it will dereference the napi_extended_error_info structure. Will it still be inlined then?

[gabrielschulhof]

/me is not too familiar with the conditions that make it impossible to inline things.

[boingoing]

I'm pretty sure napi_set_last_error() will almost always be inlined even if it dereferences the env argument. The cost of checking the existing status code may not be able to be optimized-out, though. My vote is to leave the function as-is and let the compiler inline it.

[gabrielschulhof]

Alrighty then.