Make the source a regular class #1498
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tobim
commented
Mar 25, 2021
tobim
force-pushed
the
topic/detemplate-source
branch
2 times, most recently
from
ada0243
to
409299a
Compare
|
@elvisoric you might be interested in this. It should remove some of the friction for exploratory changes. |
|
@tobim Thank you. I'll take a look. |
tobim
force-pushed
the
topic/detemplate-source
branch
from
409299a
to
6336b7f
Compare
dominiklohmann
requested changes
Mar 30, 2021
This introduces a reader factory that is used for `vast import` and `spawn source` instead of instantiating `source`, `make_source`, `import_command`, etc with every existing reader type. This change is intended to improve the dev experience during modify -> compile -> run cycles by speeding up incremental builds. A local test simulating a change to `source.hpp` gives the following times for me: Before: ``` $ touch libvast/vast/system/source.hpp $ time cmake --build ~/t/build/vast/debug -j 1 [0/2] Re-checking globbed directories... [19/19] Linking CXX executable bin/example-test ________________________________________________________ Executed in 155.45 secs fish external usr time 154.25 secs 38.12 millis 154.21 secs sys time 8.83 secs 5.00 millis 8.83 secs ``` After: ``` $ touch libvast/vast/system/source.hpp $ time cmake --build ~/t/build/vast/detemp -j 1 [0/2] Re-checking globbed directories... [15/15] Linking integration test directory ________________________________________________________ Executed in 83.25 secs fish external usr time 85.76 secs 0.00 micros 85.76 secs sys time 5.49 secs 712.00 micros 5.49 secs ```
dominiklohmann
force-pushed
the
topic/detemplate-source
branch
from
9f955b8
to
328257e
Compare
dominiklohmann
approved these changes
Apr 6, 2021
There was a problem hiding this comment.
I made the requested changes myself, they're all contained in the last commit. @tobim if you approve of my changes, please just merge the PR after CI is done.
tobim
commented
Apr 6, 2021
- Pulls in changes from upstream - Removes templated source_common utility functions
dominiklohmann
force-pushed
the
topic/detemplate-source
branch
from
328257e
to
e02de1b
Compare
tobim
commented
Apr 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
馃摂 Description
This introduces a reader factory that is used for
vast importandspawn sourceinstead of instantiatingsource,make_source,import_command, etc with every existing reader type.This change is intended to improve the dev experience during modify -> compile -> run cycles by speeding up incremental builds.
A local test simulating a change to
source.hppgives the following times for me:Before:
After:
馃幆 Review Instructions
Sorry for the single big commit, it is probably easiest to compare the this against the merge base by checking both locally and opening the header of master next to the
cppfrom this branch, i.e.master/source.hpp|detemplate/source.cpp.One can probably start by looking at the changes that were done for the
readerand derived classes, and then go from outside in:application.cpp->import_command->make_source->source&source_common.spawn_sourceandnode.cppare obvious afterwards.I'm available for a review call is anyone is interested.