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

Faster Base64 Encoding for NIOWebsocket connections #1328

Merged
merged 1 commit into from Dec 24, 2019

Conversation

@fabianfett
Copy link
Contributor

fabianfett commented Dec 23, 2019

Motivation:

In performance tests was shown that the current base 64 encoding function doesn’t perform very well.

#1322

Modifications:

This commit replaces the current Base64 implementation with a more performant one (by using UInt8 bytes instead of Unicode.Scalars).

Result:

The API will stay the same. Base64 encoding is just faster.

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

8 similar comments
@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

@swift-server-bot

This comment has been minimized.

Copy link

swift-server-bot commented Dec 23, 2019

Can one of the admins verify this patch?

Copy link
Member

weissi left a comment

Thanks so much, this looks awesome. Two comments.

Sources/NIOWebSocket/Base64.swift Show resolved Hide resolved
/// lookup table or anything intelligent like that, just shifts and masks. This works fine, for
/// now: the purpose of this encoding is to avoid round-tripping through Data, and the perf gain
/// from avoiding that is more than enough to outweigh the silliness of this code.
@inlinable

This comment has been minimized.

Copy link
@weissi

weissi Dec 23, 2019

Member

all the @inlinable in this file don't do anything because this isn't public and always consumed within the NIOWebSocket module so I think we should take them out.

@fabianfett fabianfett force-pushed the fabianfett:ff-faster-base64-encoding branch from 1787f37 to d3a6da9 Dec 23, 2019
@weissi
weissi approved these changes Dec 23, 2019
Copy link
Member

weissi left a comment

Fantastic stuff. I'm happy with this. I'll let @Lukasa approve too though first

@Lukasa Lukasa added this to the 2.13.0 milestone Dec 23, 2019
Copy link
Contributor

Lukasa left a comment

Just a few minor changes here, if you don’t mind.

"4", "5", "6", "7", "8", "9", "+", "/",
]
// This is a simplified vendored version from:
// https://github.com/fabianfett/swift-base64-kit

This comment has been minimized.

Copy link
@Lukasa

Lukasa Dec 23, 2019

Contributor

Can you update the NOTICES file to include the OSS license you’re using on your repository?

}
/// Base64 encode a collection of UInt8 to a string, without the use of Foundation.
///
/// This function performs the world's most naive Base64 encoding: no attempts to use a larger

This comment has been minimized.

Copy link
@Lukasa

Lukasa Dec 23, 2019

Contributor

This comment is no longer accurate as there is now a lookup table. We can probably remove it.

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Dec 23, 2019

@swift-nio-bot test this please

@fabianfett fabianfett force-pushed the fabianfett:ff-faster-base64-encoding branch from d3a6da9 to 8e173a9 Dec 23, 2019
@fabianfett fabianfett requested a review from Lukasa Dec 23, 2019
Copy link
Contributor

Lukasa left a comment

One very minor nit and then I think this is good to merge.

NOTICE.txt Outdated

---

This product contains a derivation of the Fabian Fett's 'Base64.swift'.

This comment has been minimized.

Copy link
@Lukasa

Lukasa Dec 24, 2019

Contributor

Nit: not “the” Fabian Fett, just Fabian Fett. 😉

This comment has been minimized.

Copy link
@fabianfett

fabianfett Dec 24, 2019

Author Contributor

@Lukasa damn it copy and paste and not reading once more. sorry. fixed now.

Motivation:

In performance tests was shown that the current base 64 encoding function doesn’t perform very well.

#1322

Modifications:

This commit replaces the current Base64 implementation with a more performant one (by using UInt8 bytes instead of Unicode.Scalars).

Result:

The API will stay the same. Base64 encoding is just faster.
@fabianfett fabianfett force-pushed the fabianfett:ff-faster-base64-encoding branch from 8e173a9 to 5fcdea8 Dec 24, 2019
@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Dec 24, 2019

@swift-nio-bot test this please

@Lukasa
Lukasa approved these changes Dec 24, 2019
Copy link
Contributor

Lukasa left a comment

Nice, looks good to me!

@Lukasa Lukasa merged commit 6534927 into apple:master Dec 24, 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
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.