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

ByteBuffer: small & medium sized string copies over 10% faster #640

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

weissi
Copy link
Member

@weissi weissi commented Oct 30, 2018

Motivation:

urbp[start ..< end] is quite a bit faster than when done in two
operations urbp.dropFirst(start).prefix(length). This improves small (4
bytes) & medium sized (24 bytes) String copies by over 10%. Good for
HTTP headers etc.

Modifications:

replace urbp.dropFirst(start).prefix(length) by urbp[start ..< start +
length]

Result:

small & medium sized String copies faster

@weissi weissi requested a review from Lukasa October 30, 2018 03:24
@weissi weissi added the 🔨 semver/patch No public API change. label Oct 30, 2018
@weissi
Copy link
Member Author

weissi commented Oct 30, 2018

spurious test error

Fatal error: Posix wrapper adds more than 20% overhead (with wrapper: 0.72994601726532, without: 0.600885033607483

@weissi
Copy link
Member Author

weissi commented Oct 30, 2018

@swift-nio-bot test this please

@Lukasa Lukasa added this to the 1.11.0 milestone Oct 30, 2018
let newBytesPtr = UnsafeMutableRawBufferPointer(rebasing: self._slicedStorageBuffer
.dropFirst(Int(index))
.prefix(capacity))
let newBytesPtr = UnsafeMutableRawBufferPointer(rebasing: self._slicedStorageBuffer[Int(index) ..< Int(index) &+ Int(capacity)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the unchecked addition? None of the others perform it.

Copy link
Member

Choose a reason for hiding this comment

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

@Lukasa just to double check I understand you are talking about &+ (no overflow check) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct

Copy link
Member

Choose a reason for hiding this comment

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

I think I am generally fine with doing it here but if we do so we should also do it in other places where we are sure overflow can never happen to be consist. So maybe we should just not do it in this PR and then open another PR with only the "unchecked" stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is mostly my point: if we believe the unchecked math here is safe we should a) do it more consistently, and b) put comments explaining why we think that, as we've done for force-unwrapping.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

benchmarks don't show that the &+ make a diff here, removing

Motivation:

urbp[start ..< end] is quite a bit faster than when done in two
operations urbp.dropFirst(start).prefix(length). This improves small (4
bytes) & medium sized (24 bytes) String copies by over 10%. Good for
HTTP headers etc.

Modifications:

replace urbp.dropFirst(start).prefix(length) by urbp[start ..< start +
length]

Result:

small & medium sized String copies faster
@weissi weissi force-pushed the jw-faster-string-blit branch from 608dcc1 to 8c8a65a Compare October 30, 2018 20:29
@Lukasa Lukasa merged commit ab69133 into apple:master Oct 30, 2018
@weissi weissi deleted the jw-faster-string-blit branch October 30, 2018 21:31
# 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