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

src: update compile cache storage structure #54291

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 9, 2024

This refactors the compile cache handler in preparation for the JS API, and updates the compile cache storage structure into:

- $NODE_COMPILE_CACHE_DIR
  - $NODE_VERION-$ARCH-$CACHE_DATA_VERSION_TAG-$UID
      - $FILENAME_AND_MODULE_TYPE_HASH.cache

This also adds a magic number to the beginning of the cache
files for verification, and returns the status, compile
cache directory and/or error message of enabling the
compile cache in a structure, which can be converted as
JS counterparts by the upcoming JS API.

Refs: #53639

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2024
@joyeecheung joyeecheung marked this pull request as ready for review August 9, 2024 19:30
@nodejs-github-bot 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. labels Aug 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 80.59701% with 13 lines in your changes missing coverage. Please review.

Project coverage is 87.08%. Comparing base (3cbeed8) to head (8f9eb07).
Report is 449 commits behind head on main.

Files with missing lines Patch % Lines
src/compile_cache.cc 81.81% 7 Missing and 1 partial ⚠️
src/env.cc 81.81% 2 Missing and 2 partials ⚠️
src/compile_cache.h 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54291      +/-   ##
==========================================
- Coverage   87.11%   87.08%   -0.03%     
==========================================
  Files         647      647              
  Lines      181773   181965     +192     
  Branches    34889    34909      +20     
==========================================
+ Hits       158347   158470     +123     
- Misses      16739    16777      +38     
- Partials     6687     6718      +31     
Files with missing lines Coverage Δ
src/env.h 97.87% <ø> (ø)
src/compile_cache.h 75.00% <0.00%> (-25.00%) ⬇️
src/env.cc 85.24% <81.81%> (-0.33%) ⬇️
src/compile_cache.cc 80.00% <81.81%> (-2.18%) ⬇️

... and 43 files with indirect coverage changes

@joyeecheung
Copy link
Member Author

cc @nodejs/startup @nodejs/cpp-reviewers

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

We need more comments in this PR. It's extremely hard to keep track of the changes.

src/compile_cache.cc Show resolved Hide resolved
src/compile_cache.h Outdated Show resolved Hide resolved
This refactors the compile cache handler in preparation for the
JS API, and updates the compile cache storage structure into:

- $NODE_COMPILE_CACHE_DIR
  - $NODE_VERION-$ARCH-$CACHE_DATA_VERSION_TAG-$UID
    - $FILENAME_AND_MODULE_TYPE_HASH.cache

This also adds a magic number to the beginning of the cache
files for verification, and returns the status, compile
cache directory and/or error message of enabling the
compile cache in a structure, which can be converted as
JS counterparts by the upcoming JS API.
@joyeecheung joyeecheung force-pushed the refactor-compile-cache branch from afb5bbc to 8f9eb07 Compare August 12, 2024 16:00
@joyeecheung
Copy link
Member Author

We need more comments in this PR. It's extremely hard to keep track of the changes.

FYI most of the changes here are preparation for https://github.com/joyeecheung/node/tree/compile-cache-api so I only sketched out what the changes are for in the commit messages, not in the comments.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 19, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 19, 2024
@nodejs-github-bot nodejs-github-bot merged commit b246f22 into nodejs:main Aug 19, 2024
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b246f22

RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
This refactors the compile cache handler in preparation for the
JS API, and updates the compile cache storage structure into:

- $NODE_COMPILE_CACHE_DIR
  - $NODE_VERION-$ARCH-$CACHE_DATA_VERSION_TAG-$UID
    - $FILENAME_AND_MODULE_TYPE_HASH.cache

This also adds a magic number to the beginning of the cache
files for verification, and returns the status, compile
cache directory and/or error message of enabling the
compile cache in a structure, which can be converted as
JS counterparts by the upcoming JS API.

PR-URL: #54291
Refs: #53639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
This refactors the compile cache handler in preparation for the
JS API, and updates the compile cache storage structure into:

- $NODE_COMPILE_CACHE_DIR
  - $NODE_VERION-$ARCH-$CACHE_DATA_VERSION_TAG-$UID
    - $FILENAME_AND_MODULE_TYPE_HASH.cache

This also adds a magic number to the beginning of the cache
files for verification, and returns the status, compile
cache directory and/or error message of enabling the
compile cache in a structure, which can be converted as
JS counterparts by the upcoming JS API.

PR-URL: #54291
Refs: #53639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants