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

Allow users to choose linker like lld to make build faster. #3460

Merged
merged 12 commits into from
Dec 21, 2021
Merged

Allow users to choose linker like lld to make build faster. #3460

merged 12 commits into from
Dec 21, 2021

Conversation

jiayuehua
Copy link
Contributor

What type of PR is this?

  • bug
  • [* ] feature
  • enhancement

What does this PR do?

user can chose linker like lld to make build more faster.

Which issue(s)/PR(s) this PR relates to?

none

Special notes for your reviewer, ex. impact of this fix, etc:

If ci can install lld before build in the future, then can make lld default linker. Now still use bfd as default.

Additional context:

none

Checklist:

  • [* ] Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

user in the future need to install lld first to link faster.

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

support lld linker and make build a litter faster

@Sophie-Xie
Copy link
Contributor

[* ] Documentation affected

--> Does this PR affect the document?

@Sophie-Xie Sophie-Xie added the ready-for-testing PR: ready for the CI test label Dec 14, 2021
@jiayuehua
Copy link
Contributor Author

[* ] Documentation affected

--> Does this PR affect the document?

Yes . In the future, user need to install lld first. And we may set lld as default linker to build fast.

@Aiee Aiee changed the title user can chose linker like lld to make build more faster. Allow users to choose linker like lld to make build faster. Dec 14, 2021
@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Dec 14, 2021
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

I think you shouldn't change the linker config, just by passing the NEBULA_USE_LINKER into cmake in configuring stage.

please see the .github/workflows/pull_request.yml content

@jiayuehua
Copy link
Contributor Author

jiayuehua commented Dec 15, 2021

I think you shouldn't change the linker config, just by passing the NEBULA_USE_LINKER into cmake in configuring stage.

please see the .github/workflows/pull_request.yml content

No, the linker config is explicit telling what kind of linker is supported. If set it to CI, Many developer will not notice it, Also Then Can Set NEBULA_USE_LINKER to wrong value like 'notvalidld'. Set it in linkerconfig file developer can know what kind of linker supported.

@yixinglu
Copy link
Contributor

I think you shouldn't change the linker config, just by passing the NEBULA_USE_LINKER into cmake in configuring stage.
please see the .github/workflows/pull_request.yml content

No, the linker config is explicit telling what kind of linker is supported. If set it to CI, Many developer will not notice it, Also Then Can Set NEBULA_USE_LINKER to wrong value like 'notvalidld'. Set it in linkerconfig file developer can know what kind of linker supported.

make sense. And only thing needing to do is just to verify the compiling in multi-platform.

@codecov-commenter
Copy link

Codecov Report

Merging #3460 (11c7704) into master (53b85dc) will increase coverage by 0.02%.
The diff coverage is 75.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3460      +/-   ##
==========================================
+ Coverage   85.19%   85.22%   +0.02%     
==========================================
  Files        1306     1307       +1     
  Lines      122158   122244      +86     
==========================================
+ Hits       104078   104179     +101     
+ Misses      18080    18065      -15     
Impacted Files Coverage Δ
src/common/expression/VariableExpression.cpp 84.84% <ø> (-0.45%) ⬇️
...rc/graph/executor/query/AppendVerticesExecutor.cpp 98.30% <ø> (ø)
src/graph/executor/query/GetEdgesExecutor.cpp 94.00% <ø> (ø)
src/graph/executor/query/GetVerticesExecutor.cpp 96.77% <ø> (ø)
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <ø> (ø)
src/graph/optimizer/rule/IndexScanRule.h 100.00% <ø> (ø)
...mizer/rule/PushLimitDownEdgeIndexRangeScanRule.cpp 20.00% <0.00%> (-0.69%) ⬇️
...raph/optimizer/rule/PushLimitDownIndexScanRule.cpp 14.81% <0.00%> (-0.57%) ⬇️
...imizer/rule/PushLimitDownTagIndexRangeScanRule.cpp 20.00% <0.00%> (-0.69%) ⬇️
src/graph/planner/plan/Query.h 97.22% <ø> (ø)
... and 70 more

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 9ea4889...11c7704. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants