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

cleanup: reduce the number of #include "env.h" and #include "env-inl.h" in the code base #27531

Closed
joyeecheung opened this issue May 2, 2019 · 18 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented May 2, 2019

Because of how Environment is used in the code base (e.g. to grab Node.js things from callbacks, to have fast access to persistent properties), Environment is used in almost every C++ file, which results in a lot of #include "env.h" and #include "env-inl.h" in the code base. However many files (mostly headers) only need a forward declaration of class Environment instead of requiring env.h or env-inl.h because they just need to know the Environment type to have Environment* in the declarations. When there is code in a header that actually uses something from Environment (so a forward declaration is not enough), the code may be moved out of the header if it is not in a hot path (i.e. it does not need to be inlined).

Reducing the number of these includes should reduce the amount of files being compiled whenever env.h/env-inl.h is touched.

@joyeecheung joyeecheung added c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. labels May 2, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented May 2, 2019

I think this is a good first issue if you know how forward declaration and C++ headers work. If nobody picks this up (say in a week) I'll do it anyway because...I touch env.h too much these days and want to spend less time waiting for recompilation ;)

@refack
Copy link
Contributor

refack commented May 2, 2019

if it is not in a hot path (i.e. it does not need to be inlined).

BTW I believe that as of C++11 this assumption is not as relevant as it used to be.
Language-wise inline has come to mean "may have multiple definitions" on the one hand, and compilers are now inlining anything and do LTO if they think it will improve performance, on the other hand.

@Jinshuo1994
Copy link

I think the instructions from @joyeecheung are very clear. Can I take up this issue? Thanks!

@joyeecheung
Copy link
Member Author

@Jinshuo1994 sure, go head!

@BeniCheni
Copy link
Contributor

Since I'm learning c++ in context of Node core, I tried out #27620 for this "good first issue" for c++ in core. Thanks.

@sarkararpan710
Copy link

Hi, This is my first time with contributing to open source and would like to know, how exactly I can help.
Is this issue still open? Any build instructions that might help me understand how I can contribute.
Thanks.

@joyeecheung
Copy link
Member Author

joyeecheung commented May 20, 2019

@sarkararpan710 There was #27620 but it did not exactly do what this issue requests. Since there has been no progress there since more than a week, I think you can take another stab at this - please take a look at #27620 (comment) about what the request is.

@BeniCheni
Copy link
Contributor

BeniCheni commented May 20, 2019

@joyeecheung, sorry about the delay to update the PR. Been busy w. work lately. I’ll close the PR, and look out for other opportunities to do PRs.

@carolinedarling
Copy link

@joyeecheung I was wondering if I could work on this issue, is it still something that needs to be worked on? I am brand new to open-source and this would be my first contribution, so please excuse if I ask something ignorant.

@sam-github
Copy link
Contributor

I suggest waiting until #27755 lands. It probably (mostly) cleans up many unnecessary includes of env-inl.h, but it doesn't deal with the problem of files that include env.h purely for its class declaration.

As a heads up, to do this, you'll need to know how a class Environment; forward-declaration can be used to replace some instance of #include "env.h".

@carolinedarling
Copy link

@sam-github Thank you very much. Will this take about 48 hours? How will I know when it lands? I appreciate your comment.

@sam-github
Copy link
Contributor

Hopefully will land by the end of day. You can probably use the 'subscribe' button on the PR to start getting updates on it.

@carolinedarling
Copy link

@sam-github Are we waiting for someone with write access to merge it with Master, am I understanding? Then, I re-build and start working? Thank you. Thank you for your patience with a new person, I am in my last quarter of a CS degree and new to git.

@alferpal
Copy link
Contributor

Does this issue still need attention? I would like to try and fix it

@joyeecheung
Copy link
Member Author

@alferpal I think this still is an issue, but maybe to a smaller extent.

BTW https://include-what-you-use.org/ can probably be useful in hunting these down but I have never tried to get it working in this code base.

@alferpal
Copy link
Contributor

Thank you for the tip, @joyeecheung .

I'll try what you suggested.

@alferpal
Copy link
Contributor

alferpal commented Oct 26, 2019

I've tried to do a first pass of the cleanup in the header files, when possible.

My intention is to make another more thorough pass using the tool @joyeecheung mentioned, but that will take a bit more time as I have to get familiar with it and so on.

Submitted the PR because I think a little improvement is always welcome, and I don't know when I'll find the time to sit and get more familiar with the codebase.

addaleax pushed a commit that referenced this issue Oct 31, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: #27531

PR-URL: #30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
DerekNonGeneric pushed a commit to DerekNonGeneric/node-samples that referenced this issue Oct 31, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: nodejs/node#27531

PR-URL: nodejs/node#30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 5, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: #27531

PR-URL: #30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@alferpal
Copy link
Contributor

alferpal commented Nov 7, 2019

I've managed to clean some more headers leftover in the code using include-what-you-use.

For those that haven't used it, it builds the project and outputs a list of suggestions for headers that should be added or removed and the total list of includes and forward declarations a file should have.

The output is something like this:

../src/api/hooks.cc should add these lines:
#include "env.h"             // for Environment
#include "node.h"            // for async_context, async_id, AtExit, EmitAsy...
#include "util.h"            // for CHECK_NOT_NULL, FIXED_ONE_BYTE_STRING
#include "v8.h"              // for Local, String, Integer, Isolate (ptr only)

../src/api/hooks.cc should remove these lines:

The full include-list for ../src/api/hooks.cc:
#include "async_wrap.h"      // for AsyncWrap
#include "env-inl.h"         // for Environment::GetCurrent, Environment::co...
#include "env.h"             // for Environment
#include "node.h"            // for async_context, async_id, AtExit, EmitAsy...
#include "node_internals.h"  // for DebugSealHandleScope
#include "node_process.h"    // for ProcessEmit
#include "util.h"            // for CHECK_NOT_NULL, FIXED_ONE_BYTE_STRING
#include "v8.h"              // for Local, String, Integer, Isolate (ptr only)

The instructions on IWYU's documentation don't manage to build node, as make will exit just after finishing building v8 due to all the suggestions thrown from IWYU.

I managed to get a full build using the following:

make -i -k -j8 CXX=/usr/bin/include-what-you-use 2>iwyu.log
IWYU outputs it's suggestions on stderr, so that's why the redirection

Leaving this hoping it helps.

targos pushed a commit that referenced this issue Nov 8, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: #27531

PR-URL: #30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 10, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: #27531

PR-URL: #30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 10, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: #27531

PR-URL: #30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 11, 2019
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Refs: #27531

PR-URL: #30133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
addaleax pushed a commit that referenced this issue Nov 28, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Nov 30, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Dec 1, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 17, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nkreeger added a commit to nkreeger/node that referenced this issue Mar 16, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: nodejs#32293
Refs: nodejs#27531
BethGriggs pushed a commit that referenced this issue Apr 7, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Apr 12, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: nodejs#32293
Refs: nodejs#27531

Fixes: nodejs#27531
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
10 participants