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.

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

os: improve tmpdir performance #54709

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 31 additions & 0 deletions benchmark/os/tmpdir.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const common = require('../common.js');
const { tmpdir } = require('os');
const assert = require('assert');

const bench = common.createBenchmark(main, {
n: [1e6],
});

function main({ n }) {
// Warm up.
const length = 1024;
const array = [];
for (let i = 0; i < length; ++i) {
array.push(tmpdir());
}

bench.start();
for (let i = 0; i < n; ++i) {
const index = i % length;
array[index] = tmpdir();
}
bench.end(n);

// Verify the entries to prevent dead code elimination from making
// the benchmark invalid.
for (let i = 0; i < length; ++i) {
assert.strictEqual(typeof array[i], 'string');
}
}
16 changes: 5 additions & 11 deletions lib/os.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const {
SymbolToPrimitive,
} = primordials;

const { safeGetenv } = internalBinding('credentials');
const { getTempDir } = internalBinding('credentials');
const constants = internalBinding('constants').os;
const isWindows = process.platform === 'win32';

Expand Down Expand Up @@ -179,24 +179,18 @@ platform[SymbolToPrimitive] = () => process.platform;
* @returns {string}
*/
function tmpdir() {
let path;
if (isWindows) {
path = process.env.TEMP ||
let path = process.env.TEMP ||
process.env.TMP ||
(process.env.SystemRoot || process.env.windir) + '\\temp';
if (path.length > 1 && StringPrototypeEndsWith(path, '\\') &&
!StringPrototypeEndsWith(path, ':\\'))
path = StringPrototypeSlice(path, 0, -1);
trivikr marked this conversation as resolved.
Show resolved Hide resolved
} else {
path = safeGetenv('TMPDIR') ||
safeGetenv('TMP') ||
safeGetenv('TEMP') ||
'/tmp';
if (path.length > 1 && StringPrototypeEndsWith(path, '/'))
path = StringPrototypeSlice(path, 0, -1);

return path;
}

return path;
return getTempDir() || '/tmp';
}
tmpdir[SymbolToPrimitive] = () => tmpdir();

Expand Down
27 changes: 27 additions & 0 deletions src/node_credentials.cc
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,31 @@ static void SafeGetenv(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(result);
}

static void GetTempDir(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

std::string dir;

// Let's wrap SafeGetEnv since it returns true for empty string.
auto get_env = [&dir, &env](std::string_view key) {
USE(SafeGetenv(key.data(), &dir, env->env_vars()));
return !dir.empty();
};

// Try TMPDIR, TMP, and TEMP in that order.
if (!get_env("TMPDIR") && !get_env("TMP") && !get_env("TEMP")) {
return;
}

if (dir.size() > 1 && dir.ends_with("/")) {
dir.pop_back();
}

args.GetReturnValue().Set(
ToV8Value(isolate->GetCurrentContext(), dir).ToLocalChecked());
}

#ifdef NODE_IMPLEMENTS_POSIX_CREDENTIALS

static const uid_t uid_not_found = static_cast<uid_t>(-1);
Expand Down Expand Up @@ -456,6 +481,7 @@ static void InitGroups(const FunctionCallbackInfo<Value>& args) {

void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(SafeGetenv);
registry->Register(GetTempDir);

#ifdef NODE_IMPLEMENTS_POSIX_CREDENTIALS
registry->Register(GetUid);
Expand All @@ -478,6 +504,7 @@ static void Initialize(Local<Object> target,
Local<Context> context,
void* priv) {
SetMethod(context, target, "safeGetenv", SafeGetenv);
SetMethod(context, target, "getTempDir", GetTempDir);

#ifdef NODE_IMPLEMENTS_POSIX_CREDENTIALS
Environment* env = Environment::GetCurrent(context);
Expand Down
Loading