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

Stream cleanup #551

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Conversation

grisumbras
Copy link
Member

Fix #213

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #551 (c1a299a) into develop (aae1863) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #551      +/-   ##
===========================================
- Coverage    99.10%   99.10%   -0.01%     
===========================================
  Files           67       67              
  Lines         6058     6040      -18     
===========================================
- Hits          6004     5986      -18     
  Misses          54       54              
Impacted Files Coverage Δ
include/boost/json/basic_parser.hpp 100.00% <ø> (ø)
include/boost/json/detail/stream.hpp 100.00% <ø> (ø)
include/boost/json/serializer.hpp 100.00% <ø> (ø)
include/boost/json/impl/serializer.ipp 96.92% <100.00%> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aae1863...c1a299a. Read the comment docs.

@cppalliance-bot
Copy link

@vinniefalco
Copy link
Member

These affect performance

@vinniefalco vinniefalco added the Performance Related to optimization of space or size label Apr 28, 2021
@grisumbras
Copy link
Member Author

Yeah, I see that. Do you know the reason? The changes should have been purely cosmetical?

@vinniefalco
Copy link
Member

The changes should have been purely cosmetical?

The local stream allows the compiler to assume there is no aliasing

@vinniefalco
Copy link
Member

@sdkrystian knows more

@sdkrystian
Copy link
Member

@grisumbras @vinniefalco is correct -- creating the local streams allows for the compiler to assume no aliasing. Without them, it must operate under the assumption that the referenced stream may be modified by writes through aliasable pointer types within the function (or ones deeper within the call stack).

@vinniefalco
Copy link
Member

In retrospect maybe we should have left a comment on the declaration for local_stream explaining intent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Related to optimization of space or size
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too many streams
4 participants