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

Clean up include headers of analytical engine #1546

Merged
merged 8 commits into from May 17, 2022

Conversation

acezen
Copy link
Collaborator

@acezen acezen commented May 11, 2022

What do these changes do?

  • Clean up headers of GAE with iwyu
  • Remove leaf header from core/config.h, put to where leaf been used
  • analytical engine building time from 82.89s to 72.26s, size from 1889376 bytes to 1815784 bytes (on my own mac with make and remove gs_proto building time)

Related issue number

Fixes #1541

@acezen acezen changed the title Clean up includes, include what you use Clean up include headers of GAE May 12, 2022
@acezen acezen force-pushed the acezen/clean-up-headers branch 2 times, most recently from 98260eb to acff41b Compare May 16, 2022 01:03
@acezen acezen changed the title Clean up include headers of GAE Clean up include headers of analytical engine May 16, 2022
@acezen acezen marked this pull request as ready for review May 16, 2022 01:22
@acezen acezen requested review from yecol and sighingnow May 16, 2022 01:22
Copy link
Collaborator

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

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

Why bl:: -> boost::leaf?

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #1546 (f4e8d1b) into main (0aa8c3c) will decrease coverage by 1.65%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1546      +/-   ##
==========================================
- Coverage   73.17%   71.51%   -1.66%     
==========================================
  Files          88       88              
  Lines        9714     9714              
==========================================
- Hits         7108     6947     -161     
- Misses       2606     2767     +161     
Impacted Files Coverage Δ
python/graphscope/tests/unittest/test_java_app.py 49.33% <0.00%> (-50.67%) ⬇️
python/graphscope/analytical/app/java_app.py 24.24% <0.00%> (-44.95%) ⬇️
python/graphscope/client/rpc.py 88.88% <0.00%> (-3.71%) ⬇️
python/graphscope/framework/utils.py 63.28% <0.00%> (-2.61%) ⬇️
python/graphscope/framework/graph_utils.py 80.15% <0.00%> (-2.39%) ⬇️
python/graphscope/framework/app.py 90.14% <0.00%> (-1.88%) ⬇️
python/graphscope/client/session.py 73.39% <0.00%> (-1.61%) ⬇️
python/graphscope/framework/graph_schema.py 62.92% <0.00%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aa8c3c...f4e8d1b. Read the comment docs.

@acezen
Copy link
Collaborator Author

acezen commented May 16, 2022

Why bl:: -> boost::leaf?

The original bl namespace is define in core/config.h, include the boost/leaf.hpp, and it would make some code include core/config.h but not use boost/leaf also include the leaf headers. That breaks the include what you use rule.
So I remove the leaf headers from config.h, put to where leaf been used and recover bl to boost::leaf.

@yecol
Copy link
Collaborator

yecol commented May 16, 2022

Why bl:: -> boost::leaf?

The original bl namespace is define in core/config.h, include the boost/leaf.hpp, and it would make some code include core/config.h but not use boost/leaf also include the leaf headers. That breaks the include what you use rule. So I remove the leaf headers from config.h, put to where leaf been used and recover bl to boost::leaf.

One of the solutions to this is defining the bl alias at the beginning of each used file.
Too long nested namespace has harmful impact on the code readability.

@yecol
Copy link
Collaborator

yecol commented May 16, 2022

Could you also report the effectiveness of this PR to speed up the building stage?
e.g., from XXs -> YYs.

@@ -13,18 +13,41 @@
* limitations under the License.
*/

#include <glog/logging.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why glog is so special, I noticed that it is in the top of the headers in many files, while gflags (I think they are similar libraries) remains in the way down.

@acezen acezen merged commit b8ee7f0 into alibaba:main May 17, 2022
@acezen acezen deleted the acezen/clean-up-headers branch May 17, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up includes to reduce the building time for analytical engine
5 participants