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

build(deps): remove some unused dependencies #3582

Merged
merged 2 commits into from Mar 26, 2024

Conversation

dimbtp
Copy link
Contributor

@dimbtp dimbtp commented Mar 26, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

It takes quite long time to build the project on my laptop, so I tried to remove
some dependencies to see whether it will improve the compile time.

From my local test with

  • command cargo build --timings after cargo clean the cache
  • 8 threads
  • 16GB memory (about 15GB free)
  • clang version 16.0.6
  • reboot system and compile directly to reduce the affect of other stuffs

before:
before
after:
after

However, I only observed very little improvement except units reduce from 1229
to 1219. The compilation will drain all my memory and sometimes crash my GUI,
so maybe better hardware can give more improvements?

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@github-actions github-actions bot added the Doc not needed This PR do not impact docs label Mar 26, 2024
@waynexia
Copy link
Member

However, I only observed very little improvement except units reduce from 1229
to 1219.

Most of the unused deps you removed in this patch are still dependent in other places so the overall compile time doesn't change a lot. But still appreciate it 💯

@dimbtp
Copy link
Contributor Author

dimbtp commented Mar 26, 2024

Only test cargo build locally 😅, try to test others to pass check.

@WenyXu
Copy link
Member

WenyXu commented Mar 26, 2024

Have you tried the mold? It only takes ~4GiB memory for cargo build.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.90%. Comparing base (bf14d33) to head (049ca06).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3582      +/-   ##
==========================================
- Coverage   85.25%   84.90%   -0.35%     
==========================================
  Files         917      917              
  Lines      152869   152892      +23     
==========================================
- Hits       130324   129818     -506     
- Misses      22545    23074     +529     

@dimbtp
Copy link
Contributor Author

dimbtp commented Mar 26, 2024

Have you tried the mold? It only takes ~4GiB memory for cargo build.

Test it and time reduces from 461.4s (7m 41.4s) to 374.0s (6m 14.0s), really boost a lot. Thanks ❤️!

What do you mean ~4GiB memory for cargo build? As compilation still drain almost all of my free memory.

@waynexia
Copy link
Member

Have you tried the mold? It only takes ~4GiB memory for cargo build.

Maybe we should mention this somewhere in our document. In my experience mold helps in many environments.

cc @tisonkun do you have any suggestions for the place?

@tisonkun
Copy link
Contributor

Maybe we should mention this somewhere in our document. In my experience mold helps in many environments.

@waynexia it can be a subtask of a revision of CONTRIBUTING.md. You can create an issue and ping me. I schedule to handle the CONTRIBUTING.md file in April; any feedback is welcome :D

@waynexia
Copy link
Member

Maybe we should mention this somewhere in our document. In my experience mold helps in many environments.

@waynexia it can be a subtask of a revision of CONTRIBUTING.md. You can create an issue and ping me. I schedule to handle the CONTRIBUTING.md file in April; any feedback is welcome :D

Ticket: #3587

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Then let's go?

@tisonkun tisonkun enabled auto-merge March 26, 2024 10:19
@tisonkun tisonkun added this pull request to the merge queue Mar 26, 2024
Merged via the queue into GreptimeTeam:main with commit dd18d8c Mar 26, 2024
19 checks passed
@dimbtp dimbtp deleted the build/remove-unused-deps branch March 26, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc not needed This PR do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants