-
Notifications
You must be signed in to change notification settings - Fork 42
Generic flat response implementation #278
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
Open
D0zee
wants to merge
11
commits into
boostorg:develop
Choose a base branch
from
D0zee:263-generic-flat-response
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7e7e3dd
Draft version without consume_one implementation
D0zee da13036
Addressed some comments
D0zee 6971fa6
reserve function
D0zee 7b59e28
Corrected and cleaner implementation with comments
D0zee 359c8bc
Call prepare_done function to form response
D0zee 621367d
Merge branch 'develop' into 263-generic-flat-response
D0zee 1e5803b
Defined done_fn_type in adapters.hpp
D0zee 3545f89
Moved implementation of push_back to general_aggregate
D0zee 79e262a
Avoid turning throwing consume_one into template
D0zee 2d866f1
Get rid of type erasion in details::prepare_done
D0zee 51add31
Added API for generic_flat_response, replaced generic_response with n…
D0zee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to avoid the creation of two
std::function
for each request (i.e.async_exec
calls) since each one represents a potential dynamic allocation. In this case however I think we can't do much better because insideasync_exec
we only have access to the type erased adapter i.e.std::function
and therefore we can't calladapter.set_done()
for example. I think theprepare_done_fn
callback is not critical though since it is small and the dynamic allocation is likely to be optimized away with SOO (small object optimization). @anarthal What do you think?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If response type is
generic_flat_response
we will return lambda with captured reference ([res]()mutable{}
). The size of this lambda is 64 bytes,std::function
will definitely keep it on heap. I suggest to capture and pass the pointergeneric_flat_response*
. Its size is 8 bytes and in this case SOO will take place during creation ofstd::function
inany_adapter
. As a result we have no heap allocations in this case.If the response type is any other SOO will take a place by default due to the fact that size of empty lambda is 1 byte.
Source: https://stackoverflow.com/a/57049013
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking about this and there seems to be a simpler way of avoiding the extra
std::function
that I haven't considered earlier. First we define a special node value innode.hpp
then call the adapter with this node when the parser is done here
Then call
set_views()
on the adapter when this node is detected hereI totally missed this possibility earlier but it looks cleaner than adding a new
std::function
? Do you mind trying this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it looks much better and we don't have to care about heap allocations. I will try it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct that this case must be handled by others adapters as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @D0zee, yes other adapters will have to handle this as well, for example
I forgot to say this in my previous comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again, there is still one problem to solve. The way I suggested above will cause the
set_view
to be called multiple times, once for each request in the pipeline. That is because the same flat response can be used to store the response of multiple requests. I am still not sure what is the simplest way to avoid that, it can be solved by adding state toset_views()
so that it does not traverse what has already been traversed.Or perhaps there is a way to wrap the adapters at other location such as here
or here
so that only the last call to
adapter(done_node, ec)
triggers aset_view()
. I think however this might be too complicated and perhaps it just simpler to let each call to set_view to traverse only what has not been set yet, as suggested above.If I have time I will review all of this to see if there is any simplification possible.