Skip to content

Remove unused headers from gsl/pointers#1153

Merged
carsonRadtke merged 3 commits intomicrosoft:mainfrom
knst:optimize-gsl-pointers-compile-time
Oct 14, 2024
Merged

Remove unused headers from gsl/pointers#1153
carsonRadtke merged 3 commits intomicrosoft:mainfrom
knst:optimize-gsl-pointers-compile-time

Conversation

@knst
Copy link
Contributor

@knst knst commented May 30, 2024 •
edited
Loading

  • forward is already declared in utility, no need to include <algorithm> which is relatively heavy
  • hash is already declared in memory, no need to bring brand-new header <system_error> for hash only.

On my machine with my customized fork of GSL it improves a compilation time of gsl/pointers drastically.

See benchmark (best time over 5 runs):

  • baseline
$ time g++ -o a.out gsl/pointers.h -O2 -g -I.
real    0m0,572s
user    0m0,461s
sys     0m0,108s
  • removed algorithm:
real    0m0,505s
user    0m0,398s
sys     0m0,107s
  • without algorithm and system_error:
real    0m0,332s
user    0m0,265s
sys     0m0,067s

Considering using gsl/pointers all over codebase it speeds up compilation time of any project that uses gsl/pointers.

@beinhaerter
Copy link
Contributor

beinhaerter commented May 30, 2024

I just see that std::greater<> and std::less<> are used, but <functional> is not included for them. Maybe this can be fixed, too?

beinhaerter
beinhaerter reviewed May 30, 2024
View reviewed changes
@knst
Remove unused headers from gsl/pointers
ead902b 
forward is already declared in utility, no need to include algorithm which is relativaly heavy
hash is already declared in memory, no need to bring brand-new header system_error for hash only
@knst knst force-pushed the optimize-gsl-pointers-compile-time branch from 7018630 to 138ac72 Compare May 30, 2024 12:28
@knst knst requested a review from beinhaerter May 30, 2024 12:30
@knst knst mentioned this pull request Jun 3, 2024
Merged
5 tasks
@knst
Copy link
Contributor Author

knst commented Jun 25, 2024

hi! Is there any chances too see a progress here? Can it get merged to the main?

it seems useful changes in my opinion.

@d-winsor d-winsor self-assigned this Jul 17, 2024
@d-winsor
Copy link
Contributor

d-winsor commented Jul 17, 2024

@knst There appears to be a problem running checks on this PR. Can you push a dummy change as shown below to retrigger the tests.

commit -m "retrigger checks" --allow-empty

@knst knst force-pushed the optimize-gsl-pointers-compile-time branch from 138ac72 to 2445d84 Compare July 18, 2024 05:29
@knst
Copy link
Contributor Author

knst commented Jul 18, 2024

@d-winsor

3 workflows awaiting approval

problem running checks on this PR

I force pushed a branch, seems as I need approval again

@PastaPastaPasta
Copy link

PastaPastaPasta commented Jul 19, 2024

@knst There appears to be a problem running checks on this PR. Can you push a dummy change as shown below to retrigger the tests.

commit -m "retrigger checks" --allow-empty

Hi @d-winsor, it appears the CI is still not triggering? Please advise.

Thanks!

@PastaPastaPasta
Copy link

PastaPastaPasta commented Aug 29, 2024

Hey all,

this has been approved for a while? CI failures seem unrelated? Please advise, it'd be nice to get some movement again

@carsonRadtke carsonRadtke merged commit 84b2ca1 into microsoft:main Oct 14, 2024
@carsonRadtke
Copy link
Member

carsonRadtke commented Oct 14, 2024

@knst Thanks for the contribution!

Apologies it took so long to get the CI issues sorted out.

@knst
Copy link
Contributor Author

knst commented Oct 15, 2024

@carsonRadtke thanks a lot for getting it merged!

@knst knst deleted the optimize-gsl-pointers-compile-time branch October 15, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gdr-at-ms gdr-at-ms gdr-at-ms approved these changes

+2 more reviewers

@d-winsor d-winsor d-winsor approved these changes

@beinhaerter beinhaerter beinhaerter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@d-winsor d-winsor

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@knst @beinhaerter @d-winsor @PastaPastaPasta @carsonRadtke @gdr-at-ms