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

refactor: reduce build time #4232

Merged
merged 22 commits into from
Oct 19, 2022
Merged

Conversation

jackgerrits
Copy link
Member

@jackgerrits jackgerrits commented Oct 18, 2022

  • Move spdlog to private dependency in vw_io by adding virtual interface for log sink
    • This has a massive impact on build time because the spdlog header is relatively heavy and caused MANY heavy template instantiations
  • Make vw_fwd header lighter
  • Split apart some other headers

On my machine my build time went from about 30s to 17s.

@@ -5,7 +5,7 @@
#include "vw/common/vw_exception.h"
#include "vw/core/reductions/cb/cb_algs.h"
#include "vw/core/scope_exit.h"
#include "vw/core/vw.h"
// #include "vw/core/vw.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this line instead of commenting it out?

@@ -2,6 +2,8 @@
// individual contributors. All rights reserved. Released under a BSD (revised)
// license as described in the file LICENSE.

#define NOMINMAX
Copy link
Collaborator

Choose a reason for hiding this comment

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

this used to be inside a ifdef _WIN32, is there a reason to not follow that

Copy link
Member Author

Choose a reason for hiding this comment

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

CLI will only build on windows as far as I know so I figured it wasnt necessary

@jackgerrits jackgerrits merged commit 0180673 into VowpalWabbit:master Oct 19, 2022
@jackgerrits jackgerrits deleted the compile_speed branch October 19, 2022 14:44
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.

4 participants