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

lib: fix code cache generation, add more tests and enable them by default #23855

Closed
wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

lib: fix code cache generation

e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

test: run code cache test by default and test generator

  • Add the code cache tests to the default test suite, and test
    the bookkeeping when the binary is not built with the code cache.
  • Test the code cache generator to make sure we do not accidentally
    break it - until we enable code cache in the CI.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Oct 24, 2018
@joyeecheung
Copy link
Member Author

assert(isUint8Array(codeCache[key]) && codeCache[key].length > 0,
`Code cache for "${key}" should've been generated`);
} else {
// The binary is not configured with code cache.
Copy link
Member

Choose a reason for hiding this comment

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

Remove "not" ?

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM with nit addressed

assert.ifError(child.error);

const stat = fs.statSync(dest);
assert(stat.isFile());
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer assert.ok
Is there anything else we can test about the file? Maybe that it has the string static uint8_t internal_bootstrap_cache_raw[]?

test/code-cache/test-code-cache-generator.js Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

Addressed nits and added tests for verifying node::DefineCodeCache() and node::DefineCodeCacheHash() are defined in the generated code.

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

@refack
Copy link
Contributor

refack commented Oct 25, 2018

Addressed nits and added tests for verifying node::DefineCodeCache() and node::DefineCodeCacheHash() are defined in the generated code.

Thank you!

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 26, 2018

Hmm, CI was not happy, I'll investigate

https://ci.nodejs.org/job/node-test-commit-linux-containered/8109/nodes=ubuntu1604_sharedlibs_withoutintl_x64/console

18:36:01   ...
18:36:02 not ok 2161 code-cache/test-code-cache-generator
18:36:02   ---
18:36:02   duration_ms: 0.551
18:36:02   severity: fail
18:36:02   exitcode: 1
18:36:02   stack: |-
18:36:02     events.js:167
18:36:02           throw er; // Unhandled 'error' event
18:36:02           ^
18:36:02     
18:36:02     Error: ENOENT: no such file or directory, open '/home/iojs/node-tmp/.tmp.1/cache.cc'
18:36:02     Emitted 'error' event at:
18:36:02         at fs.open (internal/fs/streams.js:130:12)
18:36:02         at FSReqCallback.oncomplete (fs.js:148:20)

e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.
@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 28, 2018

Rebased and added logs when the tools/generate_code_cache.js command fails (somehow when the child process fails (child.status !== 0), child.error is still null..is that a child process bug?)

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

EDIT: --without-intl implying no inspector is intentional: #12758

@joyeecheung
Copy link
Member Author

Apparently inspector is not available in the withoutintl job (cc @refack is that intentional?)..skipped the inspector modules when inspector is not available.

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

@joyeecheung
Copy link
Member Author

trace events are also unavailable in withoutintl jobs..fixed the cannotUseCache list again

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

@joyeecheung
Copy link
Member Author

So the resume build is green: https://ci.nodejs.org/job/node-test-pull-request/18204/
Although previously there were two occurrence of SSL routines:ssl3_get_record:decryption failed or bad record mac:../deps/openssl/openssl/ssl/record/ssl3_record.c:469: see #23913

I am leaning towards that's a irrelevant flake.

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 29, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 29, 2018

@refack @cjihrig @targos Can you take another look? (not too much has changed since the initial iteration, though, mostly just skipping creating cache for modules that are not available in withoutintl jobs)

@@ -42,6 +43,15 @@ const cannotUseCache = [
'internal/bootstrap/node'
].concat(depsModule);

if (process.config.variables.v8_enable_inspector !== 1) {
Copy link
Member

Choose a reason for hiding this comment

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

May be worthwhile including a quick comment with these that says why they cannot use the cache, just so it's clear to folks coming along later.

@joyeecheung
Copy link
Member Author

Landed in b255cd4...010a3f8 with the comment requested by @jasnell added

joyeecheung added a commit that referenced this pull request Oct 30, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this pull request Oct 30, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Nov 2, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Nov 2, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants