Skip to content

Conversation

@weissi
Copy link
Member

@weissi weissi commented May 5, 2018

CC @milseman / @airspeedswift / @tanner0101

Motivation:

Very unfortunately SR-7602
Swift's String doesn't work very well on UTF-8 encoded data. Fortunately
(but unfairly see point (3) in
SR-7602) however there is now a
contiguous String storage for ASCII-only Strings that only the stdlib
can access. To benefit from the stdlib's capabilities of enumerating the
bytes faster than we can, we should use UnsafeMutableBufferPointer#initialize(from: Bytes).
Thanks @airspeedswift for pointing this out.

The performance gains on a recent Swift version are noticeable:

after this patch:

no-net_http1_10k_reqs_1_conn: 0.225782990455627, 0.228317022323608, 0.225206971168518, 0.22549307346344, 0.223728060722351, 0.225827932357788, 0.222437977790833, 0.221745014190674, 0.220004916191101, 0.222965002059937
http1_10k_reqs_1_conn: 0.688782095909119, 0.682757973670959, 0.689703941345215, 0.708670020103455, 0.691922068595886, 0.687381982803345, 0.681567072868347, 0.684866905212402, 0.683283090591431, 0.699701905250549
http1_10k_reqs_100_conns: 0.782313942909241, 0.779283046722412, 0.773630023002625, 0.776782989501953, 0.774596929550171, 0.775121927261353, 0.773631930351257, 0.784304022789001, 0.795854926109314, 0.786051988601685

before this patch:

no-net_http1_10k_reqs_1_conn: 0.232797980308533, 0.236240983009338, 0.237086057662964, 0.236070990562439, 0.236560940742493, 0.236254930496216, 0.234392046928406, 0.235393047332764, 0.234088897705078, 0.245710015296936
http1_10k_reqs_1_conn: 0.76228404045105, 0.788464903831482, 0.747326016426086, 0.753327012062073, 0.735787987709045, 0.727090001106262, 0.720335006713867, 0.721843004226685, 0.719871044158936, 0.728644967079163
http1_10k_reqs_100_conns: 0.841314911842346, 0.884468078613281, 0.854345917701721, 0.847344040870667, 0.847607016563416, 0.841101050376892, 0.814589977264404, 0.818693995475769, 0.832610964775085, 0.828818082809448

Modifications:

use UnsafeMutableBufferPointer#initialize(from: Bytes) to enumerate
String's bytes.

Result:

faster but still not fast enough, we need SR-7602 to be addressed

Motivation:

Very unfortunately [SR-7602](https://bugs.swift.org/browse/SR-7602)
Swift's String doesn't work very well on UTF-8 encoded data. Fortunately
(but unfairly see point (3) in
[SR-7602](https://bugs.swift.org/browse/SR-7602)) however there is now a
contiguous String storage for ASCII-only Strings that _only the stdlib_
can access. To benefit from the stdlib's capabilities of enumerating the
bytes faster than we can, we should use UnsafeMutableBufferPointer#initialize(from: Bytes).
Thanks @airspeedswift for pointing this out.

The performance gains on a recent Swift version are noticeable:

after this patch:

```
no-net_http1_10k_reqs_1_conn: 0.225782990455627, 0.228317022323608, 0.225206971168518, 0.22549307346344, 0.223728060722351, 0.225827932357788, 0.222437977790833, 0.221745014190674, 0.220004916191101, 0.222965002059937
http1_10k_reqs_1_conn: 0.688782095909119, 0.682757973670959, 0.689703941345215, 0.708670020103455, 0.691922068595886, 0.687381982803345, 0.681567072868347, 0.684866905212402, 0.683283090591431, 0.699701905250549
http1_10k_reqs_100_conns: 0.782313942909241, 0.779283046722412, 0.773630023002625, 0.776782989501953, 0.774596929550171, 0.775121927261353, 0.773631930351257, 0.784304022789001, 0.795854926109314, 0.786051988601685
```

before this patch:

```
no-net_http1_10k_reqs_1_conn: 0.232797980308533, 0.236240983009338, 0.237086057662964, 0.236070990562439, 0.236560940742493, 0.236254930496216, 0.234392046928406, 0.235393047332764, 0.234088897705078, 0.245710015296936
http1_10k_reqs_1_conn: 0.76228404045105, 0.788464903831482, 0.747326016426086, 0.753327012062073, 0.735787987709045, 0.727090001106262, 0.720335006713867, 0.721843004226685, 0.719871044158936, 0.728644967079163
http1_10k_reqs_100_conns: 0.841314911842346, 0.884468078613281, 0.854345917701721, 0.847344040870667, 0.847607016563416, 0.841101050376892, 0.814589977264404, 0.818693995475769, 0.832610964775085, 0.828818082809448
```

Modifications:

use UnsafeMutableBufferPointer#initialize(from: Bytes) to enumerate
String's bytes.

Result:

faster but still not fast enough, we need SR-7602 to be addressed
@weissi weissi requested review from Lukasa and normanmaurer May 5, 2018 10:25
var (iterator, idx) = UnsafeMutableBufferPointer(start: base, count: underestimatedByteCount).initialize(from: bytes)
assert(idx == underestimatedByteCount)
while let b = iterator.next() {
assert(S.self != String.UTF8View.self)
Copy link
Member Author

Choose a reason for hiding this comment

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

@milseman I'm assuming String.UTF8View does never underestimate its count, right?

Copy link

Choose a reason for hiding this comment

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

Right now it is equivalent to count because that's the default behavior and it doesn't override it. That's likely to change at some point (CC @moiseev). But, I think it can give a very good underestimate by just returning the number of code units.

https://bugs.swift.org/browse/SR-7618

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @milseman. I made another PR (#392) to actually remove the assertion because I don't think we win anything having it...

@weissi weissi added the 🔨 semver/patch No public API change. label May 5, 2018
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.

No objection here.

@weissi weissi merged commit a7ed5fd into apple:master May 6, 2018
@weissi weissi deleted the jw-optimise-string-write branch May 6, 2018 08:30
@Lukasa Lukasa added this to the 1.7.0 milestone May 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants