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

bootstrap: include perf_hooks in the builtin snapshot #38971

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jun 8, 2021

perf_hooks: refactor perf_hooks for snapshot building

  • Move Performance and InternalPerformance to a new
    lib/internal/perf/performance.js
  • Move now() getMilestoneTimestamp() into
    lib/internal/perf/utils.js
  • Rename lib/internal/perf/perf.js to
    lib/internal/perf/performance_entry.js
  • Refresh time origin at startup (this means the
    time origins could differ between snapshot building
    time and snapshot creation time)

bootstrap: support perf hooks in snapshot

bootstrap: load perf_hooks eagerly during bootstrap

Refs: #35711

@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 8, 2021
@joyeecheung joyeecheung requested a review from jasnell June 8, 2021 16:40
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 10, 2021

Hmm, not sure why the windows build couldn't find the js2c output with this change, it only changed a few JS files in node.gyp (also the file name out\\Reease\\bj/global_intermediate/node_javascript.cc looks off)

 Traceback (most recent call last):
    File "D:\a\node\node\tools\js2c.py", line 222, in <module>
      main()
    File "D:\a\node\node\tools\js2c.py", line 218, in main
      JS2C(source_files, options.target)
    File "D:\a\node\node\tools\js2c.py", line 153, in JS2C
      write_if_chaged(out, target)
    File "D:\a\node\node\tools\js2c.py", line 187, in write_if_chaged
      with open(target, "wt") as output:
  FileNotFoundError: [Errno 2] No such file or directory: 'out\\Reease\\bj/global_intermediate/node_javascript.cc'
  js-heap-broker.cc
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(241,5): error MSB8066: Custom build for 'tools\js2c.py;src\inspector\node_protocol.pdl;src\inspector\node_protocol_config.json;deps\v8\include\js_protocol.pdl;out\Release\\obj\global_intermediate\concatenated_protocol.json;deps\openssl\openssl\util\libcrypto.num' exited with code 1. [D:\a\node\node\libnode.vcxproj]

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

I can reproduce the windows bug locally and upon checking the generated VS build files I noticed that the Command field in the generated js2c CustomBuild in libnode.vxproj includes all the library file names and it is extremely long, so I think my patch probably hit a bug by exceeding some length limit or something?

@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 17, 2021

When I tentatively remove one file from node.gyp (to shorten the length a bit) the action works again. I checked the length of the command and turns out it does exceed the Windows command length limit..I think we can work around it by stop passing the file names directly through command line. Will work on that later.

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

#39069 fixes the Windows bug for me locally

@richardlau richardlau mentioned this pull request Jun 24, 2021
25 tasks
jasnell pushed a commit that referenced this pull request Jun 24, 2021
On Windows there is a limit to the length of commands, so there
will be an error once the lengths of the JS file names combined
exceed that limit. This patch modifies js2c.py so that
it now takes a --directory argument to glob for .js and
.mjs files in addition to the list of files passed directly.
We still pass the additional files we include from deps/
directly through the command line, as we only includes some of
them so we cannot simply glob, but those are limited so listing
them out should be fine.

Refs: https://docs.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/command-line-string-limitation

PR-URL: #39069
Refs: #38971
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
- Move Performance and InternalPerformance to a new
  lib/internal/perf/performance.js
- Move now() getMilestoneTimestamp() into
  lib/internal/perf/utils.js
- Rename lib/internal/perf/perf.js to
  lib/internal/perf/performance_entry.js
- Refresh time origin at startup (this means the
  time origins could differ between snapshot building
  time and snapshot creation time)
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Jun 28, 2021
- Move Performance and InternalPerformance to a new
  lib/internal/perf/performance.js
- Move now() getMilestoneTimestamp() into
  lib/internal/perf/utils.js
- Rename lib/internal/perf/perf.js to
  lib/internal/perf/performance_entry.js
- Refresh time origin at startup (this means the
  time origins could differ between snapshot building
  time and snapshot creation time)

PR-URL: #38971
Refs: #35711
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Jun 28, 2021
PR-URL: #38971
Refs: #35711
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Jun 28, 2021
PR-URL: #38971
Refs: #35711
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in 2de139b...58cfca8

targos pushed a commit that referenced this pull request Jul 11, 2021
On Windows there is a limit to the length of commands, so there
will be an error once the lengths of the JS file names combined
exceed that limit. This patch modifies js2c.py so that
it now takes a --directory argument to glob for .js and
.mjs files in addition to the list of files passed directly.
We still pass the additional files we include from deps/
directly through the command line, as we only includes some of
them so we cannot simply glob, but those are limited so listing
them out should be fine.

Refs: https://docs.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/command-line-string-limitation

PR-URL: #39069
Refs: #38971
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2021
- Move Performance and InternalPerformance to a new
  lib/internal/perf/performance.js
- Move now() getMilestoneTimestamp() into
  lib/internal/perf/utils.js
- Rename lib/internal/perf/perf.js to
  lib/internal/perf/performance_entry.js
- Refresh time origin at startup (this means the
  time origins could differ between snapshot building
  time and snapshot creation time)

PR-URL: #38971
Refs: #35711
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2021
PR-URL: #38971
Refs: #35711
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2021
PR-URL: #38971
Refs: #35711
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
On Windows there is a limit to the length of commands, so there
will be an error once the lengths of the JS file names combined
exceed that limit. This patch modifies js2c.py so that
it now takes a --directory argument to glob for .js and
.mjs files in addition to the list of files passed directly.
We still pass the additional files we include from deps/
directly through the command line, as we only includes some of
them so we cannot simply glob, but those are limited so listing
them out should be fine.

Refs: https://docs.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/command-line-string-limitation

PR-URL: #39069
Refs: #38971
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants