Skip to content

module: skip NODE_COMPILE_CACHE when policy is enabled - #52577

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
joyeecheung:cache-policy
Apr 25, 2024
Merged

module: skip NODE_COMPILE_CACHE when policy is enabled#52577
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
joyeecheung:cache-policy

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Apr 18, 2024

Copy link
Copy Markdown
Member

It might be worth designing a policy for the compilation cache. For now, just skip the cache when policy is enabled.

@joyeecheung
module: skip NODE_COMPILE_CACHE when policy is enabled
0d435bc
It might be worth designing a policy for the compilation cache. For
now, just skip the cache when policy is enabled.
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2024
@nodejs-github-botnodejs-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. labels Apr 18, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 18, 2024

Copy link
Copy Markdown
Collaborator

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

@joyeecheung

joyeecheung commented Apr 18, 2024

Copy link
Copy Markdown
MemberAuthor

huh there isn't a team for policy, it seems, so cc @nodejs/startup @nodejs/loaders @nodejs/security

legendecas
legendecas reviewed Apr 18, 2024
View reviewed changes
Comment threadsrc/env.cc
return;
}
if (!options()->experimental_policy.empty()) {
Debug(this,

@legendecaslegendecasApr 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this be a warning instead?

@joyeecheungjoyeecheungApr 18, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The environment variable is designed to be as silent as possible. I don't think there's anything to warn - you don't get a speed up and that is all, your code will still work as intended. If you are using policy it's likely that you are already aware this is already leading to a slowdown for every module loaded due to all the checks required.

benjamingr
benjamingr approved these changes Apr 18, 2024
View reviewed changes

@benjamingrbenjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for following up with this!

@joyeecheungjoyeecheung mentioned this pull request Apr 18, 2024
Closed
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 22, 2024

Copy link
Copy Markdown
Collaborator

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

@joyeecheung

joyeecheung commented Apr 22, 2024

Copy link
Copy Markdown
MemberAuthor

Updated the test to skip in no crypto builds because policy requires crypto to work.

@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 22, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 22, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 23, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 23, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 24, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2024

Copy link
Copy Markdown
Collaborator

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

@joyeecheungjoyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2024
@joyeecheungjoyeecheung added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 25, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 2231be6 into nodejs:mainApr 25, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2024

Copy link
Copy Markdown
Collaborator

Landed in 2231be6

aduh95 pushed a commit that referenced this pull request Apr 29, 2024
@joyeecheung@aduh95
module: skip NODE_COMPILE_CACHE when policy is enabled
544c602
It might be worth designing a policy for the compilation cache. For
now, just skip the cache when policy is enabled.
PR-URL: #52577
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95aduh95 mentioned this pull request Apr 30, 2024
Merged
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 23, 2025
@joyeecheung
module: skip NODE_COMPILE_CACHE when policy is enabled
5ac763c
It might be worth designing a policy for the compilation cache. For
now, just skip the cache when policy is enabled.
PR-URL: nodejs#52577
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@claudeclaudeBot mentioned this pull request Jul 25, 2026
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes
@richardlaurichardlaurichardlau approved these changes
@legendecaslegendecaslegendecas approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@nodejs-github-bot@benjamingr@richardlau@legendecas@marco-ippolito