Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

shrink HTTP{Request,Response}Head by CoW boxing them #351

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

weissi
Copy link
Member

@weissi weissi commented Apr 24, 2018

only really makes sense with #349

Motivation:

HTTP{Request,Response}Head were too large even with a less than 3 word
ByteBuffer, this shrinks them box CoW boxing them. Should also reduce
ARC traffic when passing around a bunch.

Modifications:

CoW box HTTP{Request,Response}Head

Result:

fewer existential containers created, more performance

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Apr 24, 2018
@Lukasa Lukasa added this to the 1.6.0 milestone Apr 24, 2018
@weissi
Copy link
Member Author

weissi commented Apr 24, 2018

tests failed because allocations went up which is expected until #349 lands, retrying then

Motivation:

HTTP{Request,Response}Head were too large even with a less than 3 word
ByteBuffer, this shrinks them box CoW boxing them. Should also reduce
ARC traffic when passing around a bunch.

Modifications:

CoW box HTTP{Request,Response}Head

Result:

fewer existential containers created, more performance
@weissi weissi force-pushed the jw-extra-pr-for-cory branch from 0c51504 to a4c1c48 Compare April 24, 2018 17:51
@weissi
Copy link
Member Author

weissi commented Apr 24, 2018

#349 now merged, so tests should now pass

@normanmaurer normanmaurer merged commit bc61e6a into apple:master Apr 24, 2018
@normanmaurer
Copy link
Member

@weissi yep it did... so I merged this ;)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants