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

[Do not merge] Rapidjson #258

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[Do not merge] Rapidjson #258

wants to merge 5 commits into from

Conversation

Cylix
Copy link
Collaborator

@Cylix Cylix commented Mar 30, 2018

This is a first draft of using rapidjson.

Please do not merge:

  • I first want to make more testing. First, because the change is critical as it concerns the requests themselves, but also because this library has a very different memory management (which makes it powerful but also may need to be cautious), and finally because I want to make sure this change is worth it.
  • I believe this change would be worth it with some additional changes in the requests/responses design that would avoid copies. I believe if we go in the direction of rapidjson in order to reduce memory footprint (that's the whole point of that change), something that would be even more interesting is to make the requests more as a temporary buffer that can be move to the json memory pool. Same thing concerning the json itself by moving the json buffer into the response itself. This will require more thinking though.

This PR mostly opened to keep track of what is being done, making suggestions, think through and evaluate the cost of the change based on our library goals.

It is related to #252 and aims to reduce memory footprint.

@thibault-martinez thibault-martinez added P-low Priority - Low D-medium Difficulty - Medium S-do-not-merge Status - Do not merge S-waiting-on-author Status - Waiting on author C-enhancement Category - Enhancement A-utils Area - Utils S-failing-checks Status - Failing checks labels Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-utils Area - Utils C-enhancement Category - Enhancement D-medium Difficulty - Medium P-low Priority - Low S-do-not-merge Status - Do not merge S-failing-checks Status - Failing checks S-waiting-on-author Status - Waiting on author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants