FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build,win,v8: allow precompiling objects-inl.h by joaocgreis · Pull Request #21772 · nodejs/node · GitHub

/ node Public

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

Closed
joaocgreis wants to merge 2 commits into
nodejs:masterfrom
JaneaSystems:joaocgreis-I7B-pch
ConversationCommits (2)ChecksFiles changed
Closed

build,win,v8: allow precompiling objects-inl.h#21772
joaocgreis wants to merge 2 commits into
nodejs:masterfrom
JaneaSystems:joaocgreis-I7B-pch

Conversation

joaocgreis commented Jul 11, 2018

Copy link
Copy Markdown
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

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 commented Jul 11, 2018

Copy link
Copy Markdown
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 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 added dont-land-on-v6.x and removed install Issues and PRs related to the installers. labels Jul 11, 2018

joaocgreis commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

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

refack commented Jul 11, 2018

Copy link
Copy Markdown
Contributor

👍 (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 commented Jul 11, 2018

Copy link
Copy Markdown
Contributor

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 commented Jul 12, 2018

Copy link
Copy Markdown
Member

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

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member

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 commented Jul 12, 2018

Copy link
Copy Markdown
Member

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

refack commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

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 commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

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

joaocgreis commented Jul 16, 2018

Copy link
Copy Markdown
Member Author

@refack updated to use positive naming only

Trott commented Jul 24, 2018

Copy link
Copy Markdown
Member

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

Trott commented Jul 24, 2018

Copy link
Copy Markdown
Member

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

Trott approved these changes Jul 24, 2018
View reviewed changes

Trott 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if CI is green

Trott commented Jul 24, 2018

Copy link
Copy Markdown
Member

/ping @mcollina @trevnorris

mcollina approved these changes Jul 24, 2018
View reviewed changes

mcollina 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rubberstamp LGTM with green CI.

Trott commented Jul 25, 2018

Copy link
Copy Markdown
Member

@refack Does this look OK to you?

Trott commented Jul 25, 2018

Copy link
Copy Markdown
Member

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

kfarnung commented Jul 25, 2018

Copy link
Copy Markdown
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
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 commented Jul 27, 2018

Copy link
Copy Markdown
Member

Landed in 48e5b35.

Trott closed this Jul 27, 2018
targos pushed a commit that referenced this pull request Jul 31, 2018
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 mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
Trott Trott approved these changes
+4 more reviewers
refack refack approved these changes
digitalinfinity digitalinfinity approved these changes
kfarnung kfarnung approved these changes
bzoz bzoz approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL