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.

Sign up for GitHub

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

Jump to bottom

build,win,v8: allow precompiling objects-inl.h #21772

Closed
wants to merge 2 commits into from
Closed

build,win,v8: allow precompiling objects-inl.h #21772

wants to merge 2 commits into from

Conversation

joaocgreis
Copy link
Member

This makes compiling v8_base much faster on Windows. On my machine, the total node build time drops from 25m29.597s to 10m58.554s.

This is enabled by default when using calling vcbuild without release or build-release (similar to ltcg). All node tests pass, but this force-includes objects-inl.h in every file for v8_base, so it is possible that this causes subtle issues and is thus disabled in CI and for releases. This includes only objects-inl.h to minimize the chance for issues, it is already included in many files. Adding other headers only improves the build time in the order of seconds.

Sharding is disabled because the header would have to be precompiled for each shard but is only once. The library is much smaller, so sharding is not necessary with this either way.

Can the changes in v8.gyp be floated on top of V8 updates, or does this need to be submitted upstream? Are we maintaining the gyp files? (cc @nodejs/v8-update)

cc @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joaocgreis
build,win,v8: allow precompiling objects-inl.h
342dab7 
This makes compiling v8_base much faster on Windows.

Sharding is disabled because the header would have to be precompiled
for each shard but is only once. The library is much smaller, so
sharding is unnecessary.

This is enabled by default, but disabled for CI and releases.
@nodejs-github-bot
Copy link
Collaborator

@joaocgreis build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/254/pipeline

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform. labels Jul 11, 2018
@joaocgreis joaocgreis added dont-land-on-v6.x and removed install Issues and PRs related to the installers. labels Jul 11, 2018
@joaocgreis
Copy link
Member Author

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/15815/

@refack
Copy link
Contributor

refack commented Jul 11, 2018

👍 (I was just reading a post about the improvements in PCH and was wondering how we could benefit from that :)

I'm just confused about the positive/negative naming, especially

node/configure

Line 980 in 342dab7

o['variables']['node_use_pch'] = b(not options.without_pch)

IMHO if we want this to be the default, the flag should be consistently without for opt-out. If we want this to be a dev-only optimization, then it should be consistently with for opt-in.

/CC @nodejs/build-files

@refack
Copy link
Contributor

refack commented Jul 11, 2018

Can the changes in v8.gyp be floated on top of V8 updates, or does this need to be submitted upstream? Are we maintaining the gyp files?

AFAIK we now maintain deps/v8/gypfiles. But if this has such a benefit on compile time, maybe this could be submitted upstream as a whole?

@targos
Copy link
Member

targos commented Jul 12, 2018

We own deps/v8/gypfiles. The change wouldn't need to be floated.

@jasnell
Copy link
Member

jasnell commented Jul 12, 2018

Sidenote: given that we own deps/v8/gypfiles, would it make more sense / be cleaner for us to move those out of the deps folder?

@targos
Copy link
Member

targos commented Jul 12, 2018

@jasnell We didn't do that for our other dependencies. There are no issues with deps/v8/gypfiles from my perspective.

@refack
Copy link
Contributor

refack commented Jul 12, 2018

There are no issues with deps/v8/gypfiles from my perspective.

@targos IMHO there the small issue of "implicit assumption by other collaborators" as seen above... It's not obvious who is in charge of those files... (I'll try to submit a PR for moving those, and we could discuss further)

@refack
Copy link
Contributor

refack commented Jul 12, 2018

RE CI: linter job was stuck. Other jobs are green & other linter jobs are green.

@joaocgreis
Copy link
Member Author

@refack updated to use positive naming only

@Trott
Copy link
Member

Trott commented Jul 24, 2018

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

@Trott
Copy link
Member

Trott commented Jul 24, 2018

I imagine it would be very useful to have this in place before Code-and-Learn in October. @nodejs/code-and-learn

Trott
Trott approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@Trott
Copy link
Member

Trott commented Jul 24, 2018

/ping @mcollina @trevnorris

mcollina
mcollina approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Rubberstamp LGTM with green CI.

@Trott
Copy link
Member

Trott commented Jul 25, 2018

@refack Does this look OK to you?

@Trott
Copy link
Member

Trott commented Jul 25, 2018

Maybe @digitalinfinity @kfarnung @bzoz might have opinions too.

@kfarnung
Copy link
Contributor

I don't see any issues here. ChakraCore already uses PCH on Windows, so node-chakracore can just ignore this flag.

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 27, 2018
@joaocgreis @Trott
build,win,v8: allow precompiling objects-inl.h
48e5b35 
This makes compiling v8_base much faster on Windows.

Sharding is disabled because the header would have to be precompiled
for each shard but is only once. The library is much smaller, so
sharding is unnecessary.

This is enabled by default, but disabled for CI and releases.

PR-URL: nodejs#21772
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
@Trott
Copy link
Member

Trott commented Jul 27, 2018

Landed in 48e5b35.

@Trott Trott closed this Jul 27, 2018
targos pushed a commit that referenced this pull request Jul 31, 2018
@joaocgreis @targos
build,win,v8: allow precompiling objects-inl.h
e0f3d57 
This makes compiling v8_base much faster on Windows.

Sharding is disabled because the header would have to be precompiled
for each shard but is only once. The library is much smaller, so
sharding is unnecessary.

This is enabled by default, but disabled for CI and releases.

PR-URL: #21772
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
@targos targos mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzoz bzoz bzoz approved these changes

@kfarnung kfarnung kfarnung approved these changes

@Trott Trott Trott approved these changes

@digitalinfinity digitalinfinity digitalinfinity approved these changes

@refack refack refack approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@joaocgreis @nodejs-github-bot @refack @targos @jasnell @Trott @kfarnung @mcollina @digitalinfinity @bzoz