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

src: improve node:os userInfo performance #55719

Merged
merged 1 commit into from
Nov 8, 2024
+28 −24
Merged

src: improve node:os userInfo performance #55719

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
5 changes: 0 additions & 5 deletions lib/os.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,11 +358,6 @@ function userInfo(options) {
if (user === undefined)
throw new ERR_SYSTEM_ERROR(ctx);

if (isWindows) {
user.uid |= 0;
user.gid |= 0;
}

return user;
}

Expand Down
47 changes: 28 additions & 19 deletions src/node_os.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,21 +287,29 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
encoding = UTF8;
}

const int err = uv_os_get_passwd(&pwd);

if (err) {
if (const int err = uv_os_get_passwd(&pwd)) {
CHECK_GE(args.Length(), 2);
env->CollectUVExceptionInfo(args[args.Length() - 1], err,
"uv_os_get_passwd");
return args.GetReturnValue().SetUndefined();
}

auto free_passwd = OnScopeLeave([&]() { uv_os_free_passwd(&pwd); });
auto free_passwd = OnScopeLeave([&] { uv_os_free_passwd(&pwd); });

Local<Value> error;

#ifdef _WIN32
Local<Value> uid = Number::New(
env->isolate(),
static_cast<double>(static_cast<int32_t>(pwd.uid & 0xFFFFFFFF)));
Copy link
Member

@lemire lemire Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This has ben discussed and I don't want to block this... but this code might be too complicated. The following might suffice:

Suggested change
static_cast<double>(static_cast<int32_t>(pwd.uid & 0xFFFFFFFF)));
static_cast<double>(static_cast<int32_t>(pwd.uid)));

So that I understand... we start with this...

typedef struct uv_passwd_s {
    char* username;
    long uid;
    long gid;
    char* shell;
    char* homedir;
} uv_passwd_t;

(source: libuv documentation.)

node/deps/uv/include/uv.h

Lines 1212 to 1218 in a243225

struct uv_passwd_s {
char* username;
unsigned long uid;
unsigned long gid;
char* shell;
char* homedir;
};

Under non-Windows systems.

But, for extra fun... under Windows, we have a different definition...

struct uv_passwd_s
{
  char *username;
  unsigned long uid;
  unsigned long gid;
  char *shell;
  char *homedir;
};

https://github.com/symplely/uv-ffi/blob/8e225f11537f9b34df7030652d00e1343d457ab1/headers/uv_windows.h#L2997C1-L3004C3

A long in Visual Studio is effectively an int32_t, it goes from -2,147,483,648 to 2,147,483,647. Whereas an unsigned long is an uint32_t.

Most other compiler systems will have long be either int32_t or int64_t (depending on whether we have a 32-bit or 64-bit system). They will have long be either uint32_t or uint64_t.

Now, Number::New's function signature takes a double. So the cast to double is mostly just to show what is happening (as it would happen implicitly).

So, setting aside the cast to double, we are doing...

uint32_t x =...
x = static_cast<int32_t>(x & 0xFFFFFFFF)

(If I got something wrong, please tell me.)

What is the purpose of the 0xFFFFFFFF?

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed these reviews. I'll open another pull-request to apply the changes.

Copy link
Member

Choose a reason for hiding this comment

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

@anonrig That's fine. I think your code is correct. It was just nitpicking.

Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of & 0xFFFFFFFF here was to mask higher bits. Unless my knowledge is horribly outdated, casting with overflow here would be UB (even though i doubt existence of implementations that don't just discard extra bits).

No strong opinion on casting to double, except for 'explicit > implicit' rule of thumb.

Copy link
Member

Choose a reason for hiding this comment

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

The purpose of & 0xFFFFFFFF here was to mask higher bits. Unless my knowledge is horribly outdated, casting with overflow here would be UB

An unsigned long under Visual Studio is a 32-bit unsigned integer. So we are casting an uint32 to an int32.

https://learn.microsoft.com/en-us/cpp/cpp/data-type-ranges?view=msvc-170

Node is C++20 and in C++20 casting from unsigned to signed and back is well defined.

No strong opinion on casting to double, except for 'explicit > implicit' rule of thumb.

I do not care at all. I only remarked that @anonrig's code was not perfectly consistent. I did not ask for the code to change.

Local<Value> gid = Number::New(
env->isolate(),
static_cast<double>(static_cast<int32_t>(pwd.gid & 0xFFFFFFFF)));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static_cast<double>(static_cast<int32_t>(pwd.gid & 0xFFFFFFFF)));
static_cast<double>(static_cast<int32_t>(pwd.gid)));

#else
Local<Value> uid = Number::New(env->isolate(), pwd.uid);
Copy link
Member

Choose a reason for hiding this comment

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

The cast to double is implicit, but for Windows you make it explicit... if so then I would suggest making it explicit here:

Suggested change
Local<Value> uid = Number::New(env->isolate(), pwd.uid);
Local<Value> uid = Number::New(env->isolate(), static_cast<double>(pwd.uid));

Local<Value> gid = Number::New(env->isolate(), pwd.gid);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Local<Value> gid = Number::New(env->isolate(), pwd.gid);
Local<Value> gid = Number::New(env->isolate(), static_cast<double>(pwd.gid));

#endif

MaybeLocal<Value> username = StringBytes::Encode(env->isolate(),
pwd.username,
encoding,
Expand All @@ -323,21 +331,22 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
return;
}

Local<Object> entry = Object::New(env->isolate());

entry->Set(env->context(), env->uid_string(), uid).Check();
entry->Set(env->context(), env->gid_string(), gid).Check();
entry->Set(env->context(),
env->username_string(),
username.ToLocalChecked()).Check();
entry->Set(env->context(),
env->homedir_string(),
homedir.ToLocalChecked()).Check();
entry->Set(env->context(),
env->shell_string(),
shell.ToLocalChecked()).Check();

args.GetReturnValue().Set(entry);
constexpr size_t kRetLength = 5;
std::array<Local<v8::Name>, kRetLength> names = {env->uid_string(),
env->gid_string(),
env->username_string(),
env->homedir_string(),
env->shell_string()};
std::array values = {uid,
gid,
username.ToLocalChecked(),
homedir.ToLocalChecked(),
shell.ToLocalChecked()};
args.GetReturnValue().Set(Object::New(env->isolate(),
Null(env->isolate()),
names.data(),
values.data(),
kRetLength));
}


Expand Down
Loading