Skip to content

Add a subset of boost#8492

Merged
2 commits merged intomicrosoft:mainfrom
Austin-Lamb:user/austinl/addBoost
Dec 5, 2020
Merged

Add a subset of boost#8492
2 commits merged intomicrosoft:mainfrom
Austin-Lamb:user/austinl/addBoost

Conversation

@Austin-Lamb
Copy link
Contributor

@Austin-Lamb Austin-Lamb commented Dec 4, 2020

In my #8489 we want to use boost's small_vector type, but that PR is
kinda messy by adding boost and also making a meaningful change. So
here I'm splitting out the boost addition to its own PR so that one can
be more focused on the allocation improvement and consumption of boost.

Copy link
Member

@zadjii-msft zadjii-msft 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 we're all cool with this, thanks for splitting it up!

## What should be done to update this in the future?

1. Download the version of boost you want from boost.org.
2. Take the parts you want, but leave most of it behind since it's HUGE and will bloat the repo to take it all. At the time of this writing, we only use small_vector.hpp and its dependencies as a header-only library.
Copy link
Member

Choose a reason for hiding this comment

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

we only use small_vector.hpp and its dependencies

holy shit it's 449 files for one header?! Man boost is giving npm a run for it's money

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, boost is...big. The full boost zip, when unzipped, is over 600MB - so I tried to strip it down to a minimal set. It could probably be more minimal than this but it felt hard to go smaller than whole folders of headers.

Copy link
Member

Choose a reason for hiding this comment

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

holy what

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like boost needs C++20 modules real bad.

@Austin-Lamb
Copy link
Contributor Author

I don't understand the test failure in the pipeline - these files aren't even used in the build, so how is that possible? Is this a flakey test? It seems stable from previous pipeline results from what I glanced at - so I'm just confused. Is there a way to re-run the test that failed here, @miniksa / @DHowett ?

@Austin-Lamb Austin-Lamb marked this pull request as ready for review December 4, 2020 14:04
@zadjii-msft
Copy link
Member

Don't worry too much about the CI failures - we've been having a lot of troubles with our feature tests in CI lately. I've kicked the build again, but this really doesn't need a build.

@zadjii-msft zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Meta The product is the management of the products. labels Dec 4, 2020
@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 4, 2020

I don't understand the test failure in the pipeline - these files aren't even used in the build, so how is that possible? Is this a flakey test? It seems stable from previous pipeline results from what I glanced at - so I'm just confused. Is there a way to re-run the test that failed here, @miniksa / @DHowett ?

@Austin-Lamb - the tests are flaky lately. Usually the team re-triggers them in the backend.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

K. So I really only reviewed the readme/notice/cgmanifest as the rest should just be a drop. Looks fine from that aspect. Thanks for breaking this out.

@miniksa
Copy link
Member

miniksa commented Dec 4, 2020

@msftbot, merge this after @DHowett signs off

@DHowett
Copy link
Member

DHowett commented Dec 5, 2020

I hate that boost is so huge that they ship a SEPARATE TOOL that goes and parses their includes to emit a streamlined subset of the headers that you need. I hate boost with a fiery passion for its unending complexity. At the same time, I'm forced to concede that it is useful and necessary and every C++ project inevitably ships a dependency on it.

Let's do this.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 5, 2020
@ghost
Copy link

ghost commented Dec 5, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit ddfb6ad into microsoft:main Dec 5, 2020
@vadimkantorov
Copy link

I hope this does not inflate binary size / startup time considerably

@DHowett
Copy link
Member

DHowett commented Dec 9, 2020

@vadimkantorov this PR is in service of #8489, which has some stats to back it up. It’s worth a read if you’re concerned about this change being a net negative for performance.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Product-Meta The product is the management of the products.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants