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

Cow-box WebSocketFrame. #1211

Merged
merged 2 commits into from Nov 8, 2019

Conversation

@marlimox
Copy link
Contributor

marlimox commented Oct 30, 2019

Motivation:

WebSocketFrameDecoder makes an allocation due to WebSocketFrame not fitting in an
existential. We should reduce this overhead.

Modifications:

Moved data and extensionData into a cow-box and reorder fields to more efficiently
pack them.

Result:

WebSocketFrame is now 14 bytes, down from 55.

@Lukasa Lukasa requested review from Lukasa, weissi and glbrntt Oct 30, 2019
@Lukasa Lukasa added this to the 2.10.0 milestone Oct 30, 2019
@Lukasa
Lukasa approved these changes Oct 30, 2019
Copy link
Contributor

Lukasa left a comment

This looks great to me, thanks!

Copy link
Contributor

glbrntt left a comment

Thanks Marli!

@weissi
weissi approved these changes Oct 30, 2019
Copy link
Member

weissi left a comment

Thank you! That looks great.

Copy link
Contributor

Lukasa left a comment

A few nits here.

Sources/NIOWebSocket/WebSocketFrame.swift Outdated Show resolved Hide resolved
Sources/NIOWebSocket/WebSocketFrame.swift Outdated Show resolved Hide resolved
Sources/NIOWebSocket/WebSocketFrame.swift Outdated Show resolved Hide resolved
@marlimox marlimox force-pushed the marlimox:wsfd_storage branch 3 times, most recently from 43a9192 to 36bf8a3 Oct 30, 2019
@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Oct 31, 2019

Hey @marlimox, great work! We need to drop the threshold on the allocation counter test now, as it wants us to lower the watermark for allocations so we don’t regress it.

@weissi weissi requested a review from Lukasa Oct 31, 2019
@marlimox

This comment has been minimized.

Copy link
Contributor Author

marlimox commented Nov 1, 2019

Ah yep.

@marlimox marlimox force-pushed the marlimox:wsfd_storage branch 2 times, most recently from 9b58083 to 08dcb9f Nov 1, 2019
@Lukasa
Lukasa approved these changes Nov 1, 2019
Copy link
Contributor

Lukasa left a comment

🎉

Copy link
Contributor

Lukasa left a comment

Sorry @marlimox, turns out this patch was too good: it improves a bunch of the other allocation benchmarks! Do you mind updating the following lines in each docker-compose file you’ve already edited?

MAX_ALLOCS_ALLOWED_encode_1000_ws_frames_holding_buffer=2010 should become =1010.
MAX_ALLOCS_ALLOWED_encode_1000_ws_frames_holding_buffer_with_space=2010 should become =1010.

Thanks!

Marli Oshlack
Motivation:

WebSocketFrameDecoder makes an allocation due to WebSocketFrame not fitting in an
existential. We should reduce this overhead.

Modifications:

Moved data and extensionData into a cow-box and reorder fields to more efficiently
pack them.

Result:

WebSocketFrame is now 14 bytes, down from 55.
@marlimox marlimox force-pushed the marlimox:wsfd_storage branch from 0c351f7 to 25cfae8 Nov 6, 2019
@weissi weissi requested a review from Lukasa Nov 7, 2019
@weissi weissi modified the milestones: 2.10.0, 2.11.0 Nov 7, 2019
@Lukasa
Lukasa approved these changes Nov 8, 2019
Copy link
Contributor

Lukasa left a comment

Sweet, this patch looks perfect. Thanks @marlimox!

@Lukasa Lukasa merged commit e4356bd into apple:master Nov 8, 2019
4 checks passed
4 checks passed
pull request validation (5.0) Build finished.
Details
pull request validation (5.1) Build finished.
Details
pull request validation (api breakage) Build finished.
Details
pull request validation (sanity) Build finished.
Details
@marlimox

This comment has been minimized.

Copy link
Contributor Author

marlimox commented Nov 8, 2019

Thank you for the help! 😄

@marlimox marlimox deleted the marlimox:wsfd_storage branch Nov 8, 2019
@weissi weissi modified the milestones: 2.11.0, 2.10.1 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.