src: improve `node:os` userInfo performance #55719

anonrig · 2024-11-04T16:11:26Z

Improve node:os userInfo performance by reducing the cost of creating a v8::Object.

codecov · 2024-11-04T17:44:44Z

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (bdc2662) to head (6d0bf5a).
Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_os.cc	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55719      +/-   ##
==========================================
+ Coverage   88.38%   88.41%   +0.02%     
==========================================
  Files         654      654              
  Lines      187575   187759     +184     
  Branches    36096    36126      +30     
==========================================
+ Hits       165795   166012     +217     
+ Misses      15004    14996       -8     
+ Partials     6776     6751      -25

Files with missing lines	Coverage Δ
lib/os.js	`97.31% <ø> (-0.04%)`	⬇️
src/node_os.cc	`82.37% <94.11%> (+0.06%)`	⬆️

... and 34 files with indirect coverage changes

src/node_os.cc

nodejs-github-bot · 2024-11-05T19:38:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63433/

nodejs-github-bot · 2024-11-08T16:32:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63491/

nodejs-github-bot · 2024-11-08T18:50:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63493/

lemire · 2024-11-08T19:33:01Z

src/node_os.cc

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


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?

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

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

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.

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.

lemire · 2024-11-08T19:33:24Z

src/node_os.cc

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


Suggested change

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

static_cast<double>(static_cast<int32_t>(pwd.gid)));

lemire · 2024-11-08T19:34:06Z

src/node_os.cc

+  Local<Value> gid = Number::New(
+      env->isolate(),
+      static_cast<double>(static_cast<int32_t>(pwd.gid & 0xFFFFFFFF)));
+#else
  Local<Value> uid = Number::New(env->isolate(), pwd.uid);


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));

lemire · 2024-11-08T19:34:29Z

src/node_os.cc

+  Local<Value> gid = Number::New(
+      env->isolate(),
+      static_cast<double>(static_cast<int32_t>(pwd.gid & 0xFFFFFFFF)));
+#else
  Local<Value> uid = Number::New(env->isolate(), pwd.uid);
  Local<Value> gid = Number::New(env->isolate(), pwd.gid);


Suggested change

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

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

nodejs-github-bot · 2024-11-08T19:40:26Z

Landed in 58a8eb4

PR-URL: #55719 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Richard Lau <[email protected]>

anonrig requested a review from jasnell November 4, 2024 16:11

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. labels Nov 4, 2024

RedYetiDev added the needs-benchmark-ci PR that need a benchmark CI run. label Nov 4, 2024

anonrig requested review from ronag, lemire and joyeecheung November 4, 2024 20:41

lemire reviewed Nov 5, 2024

View reviewed changes

src/node_os.cc Outdated Show resolved Hide resolved

ronag approved these changes Nov 5, 2024

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2024

src: improve node:os userInfo performance

6d0bf5a

anonrig force-pushed the yagiz/improve-node-os branch from 55614e5 to 6d0bf5a Compare November 8, 2024 16:30

anonrig requested review from ronag, LiviaMedeiros, aduh95, richardlau and lemire November 8, 2024 16:31

aduh95 approved these changes Nov 8, 2024

View reviewed changes

richardlau approved these changes Nov 8, 2024

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2024

lemire reviewed Nov 8, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2024

nodejs-github-bot merged commit 58a8eb4 into nodejs:main Nov 8, 2024
55 checks passed

aduh95 pushed a commit that referenced this pull request Nov 16, 2024

src: improve node:os userInfo performance

16786a6

PR-URL: #55719 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: improve `node:os` userInfo performance #55719

src: improve `node:os` userInfo performance #55719

anonrig commented Nov 4, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

nodejs-github-bot commented Nov 5, 2024

nodejs-github-bot commented Nov 8, 2024

nodejs-github-bot commented Nov 8, 2024

lemire Nov 8, 2024 •

edited

Loading

anonrig Nov 8, 2024

lemire Nov 8, 2024

LiviaMedeiros Nov 8, 2024

lemire Nov 8, 2024

lemire Nov 8, 2024

lemire Nov 8, 2024

lemire Nov 8, 2024

nodejs-github-bot commented Nov 8, 2024

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

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

	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);
	Local<Value> gid = Number::New(env->isolate(), static_cast<double>(pwd.gid));

src: improve node:os userInfo performance #55719

src: improve node:os userInfo performance #55719

Conversation

anonrig commented Nov 4, 2024

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Nov 5, 2024

nodejs-github-bot commented Nov 8, 2024

nodejs-github-bot commented Nov 8, 2024

lemire Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

anonrig Nov 8, 2024

Choose a reason for hiding this comment

lemire Nov 8, 2024

Choose a reason for hiding this comment

LiviaMedeiros Nov 8, 2024

Choose a reason for hiding this comment

lemire Nov 8, 2024

Choose a reason for hiding this comment

lemire Nov 8, 2024

Choose a reason for hiding this comment

lemire Nov 8, 2024

Choose a reason for hiding this comment

lemire Nov 8, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 8, 2024

src: improve `node:os` userInfo performance #55719

src: improve `node:os` userInfo performance #55719

codecov bot commented Nov 4, 2024 •

edited

Loading

lemire Nov 8, 2024 •

edited

Loading