[stdlib] Rewrite UTF8 decoding #1525

PatrickPijnappel · 2016-03-03T12:52:05Z

What's in this pull request?

Expanding upon #1477, this is a thorough rewrite of UTF8.decode() for optimization, code simplification and clarity.

Merged maximal subpart determination and conversion to code point with _isValidUTF8() (renamed to _decodeOne()), significantly reducing total amount of code and eliminating unnecessary branching.
Very significant ASCII decoding speed-up by skipping the buffer.
Overall performance improvement, branch elimination and code simplification.
Added several clarification comments throughout.
Expanded validation testing to count sequence lengths and maximal subparts.
Validated against original implementation for all inputs up to 4 code units (i.e. all 2^32 possibilities).

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

Based on the API Design Guidelines.

_atEnd was confusing because it did not mean 'at the end of the sequence', because of buffering.

dabrahams · 2016-03-08T00:19:46Z

@PatrickPijnappel Hi; can you please merge master into your branch and resolve conflicts? Thanks.

dabrahams · 2016-03-08T00:28:14Z

stdlib/public/core/Unicode.swift

-  /// - parameter next: A generator of code units to be decoded.
+  /// - Parameter next: A generator of code units to be decoded.  Repeated
+  /// calls to this method on the same instance should always reuse the same
+  /// generator.  Failing to do so will result in undefined behavior.


Please indent following lines of a bullet by two spaces.
What do you mean by “reuse” here? Would “pass” be a better word?

I don’t think you mean undefined behavior; that actually includes memory and type-safety violations. I think you mean the results are unspecified. I think you also want to say that the generator or copies thereof shouldn’t be used for anything else between calls to this method. This seems like a complicated-enough restriction to warrant an example of correct usage.

PatrickPijnappel · 2016-03-13T10:03:38Z

@dabrahams Resolved conflicts and fixed documentation.

These methods would be much more self-explanatory if we just had:

static func decode<S where S.Generator.Element == CodeUnit>(sequence: S) -> SomeSequence<UnicodeScalar>
static func encode<S where S.Generator.Element == UnicodeScalar>(sequence: S) -> SomeSequence<CodeUnit>

Is there a specific reason for the current design to be preferable?

dabrahams · 2016-03-13T13:13:48Z

@swift-ci Please test

PatrickPijnappel · 2016-03-14T01:10:35Z

@dabrahams Please test!

gottesmm · 2016-03-14T05:15:32Z

@swift-ci Please test

rintaro · 2016-03-14T10:12:53Z

stdlib/public/core/StringUTF8.swift

-        return UTF8._isValidUTF8(UInt32(truncatingBitPattern: _buffer)) || _isAtEnd
+        let buffer = UInt32(truncatingBitPattern: _buffer)
+        let (codePoint, _) = UTF8._decodeOne(buffer)
+        return codePoint != nil || _isAtEnd
      }


@PatrickPijnappel
Just out of curiosity, why return !UTF8.isContinuation(UTF8.CodeUnit(truncatingBitPattern: _buffer)) is insufficient?

@rintaro That would change the behavior slightly w.r.t. the original version, as not all non-continuation code units are valid starting code units (e.g. 0xff). Though the difference is somewhat inconsequential as _buffer should be well-formed, and in general it's not clear from the documentation what the handling of mal-formed sequences should be.

Thanks for clarifying :)

PatrickPijnappel · 2016-03-14T22:42:35Z

@gottesmm I can't reproduce these test failures on my machine (using build-script -RcT) and they don't seem to be related to the PR. Any suggestions?

dabrahams · 2016-03-15T19:46:35Z

@swift-ci Please test

dabrahams · 2016-03-15T19:47:41Z

on Mon Mar 14 2016, Patrick Pijnappel <notifications-AT-i.8713187.xyz> wrote:

@gottesmm I can't reproduce these test failures on my machine (using
build-script -RcT) and they don't seem to be related to the PR. Any
suggestions?

I'm re-testing. Upstream seems to be healthy now, but maybe this test
ran at a time that it was broken.

-Dave

PatrickPijnappel · 2016-03-16T04:58:37Z

@dabrahams Ok, now OS X fails but Linux passes. Still seems unrelated to the changes though.

dabrahams · 2016-03-16T16:13:47Z

This looks like it was a failure in upstream: https://ci.swift.org/job/oss-swift_tools-RA_stdlib-DA_test-simulator/790/consoleFull#-1806112821fca400bf-2f4a-462e-b517-e058d770b2d7

dabrahams · 2016-03-16T16:17:13Z

stdlib/public/core/Unicode.swift

@@ -63,13 +64,17 @@ public protocol UnicodeCodec {
  /// Because of buffering, it is impossible to find the corresponding position
  /// in the iterator for a given returned `UnicodeScalar` or an error.
  ///
-  /// - parameter next: An iterator over the code units to be decoded.
+  /// - Parameter next: A generator of code units to be decoded.  Repeated


Please change "generator" to "iterator"

Actually I also found a few stray cases of generator in the rest of the stdlib, see #1705.

dabrahams · 2016-03-16T16:24:49Z

stdlib/public/core/Unicode.swift

@@ -829,7 +724,7 @@ extension UTF16 {
    _precondition(width(x) == 2)
    return UTF16.CodeUnit(
      (x.value - 0x1_0000) & (((1 as UInt32) << 10) - 1)
-    ) + 0xDC00
+      ) + 0xDC00


Please unindent 2 spaces

dabrahams · 2016-03-16T16:26:32Z

We're really close to being able to accept this one!

PatrickPijnappel · 2016-03-16T19:47:49Z

@dabrahams Cool :).

gribozavr · 2016-03-22T18:18:53Z

@swift-ci Please test

[stdlib] Rewrite UTF8 decoding

49741cf

gribozavr assigned dabrahams Mar 3, 2016

PatrickPijnappel added 6 commits March 4, 2016 09:53

[stdlib] Improve UTF8._decodeOne() doc comment

25b5028

Based on the API Design Guidelines.

[stdlib] Improve comments in UTF8

f5d7bd2

[stdlib] Clarify variable name in UTF8

17124c8

_atEnd was confusing because it did not mean 'at the end of the sequence', because of buffering.

[stdlib] Fix bitshift operand mask in UTF8.decode()

500dc9d

[stdlib] Copy warning to docs of all UnicodeCodecTypes

c109bb2

[stdlib] Minor refactor for clarity in UTF8.decode()

636423c

dabrahams reviewed Mar 8, 2016
View reviewed changes

PatrickPijnappel added 2 commits March 13, 2016 14:08

[stdlib] Resolve merge conflict in Unicode.swift

4d05dc6

[stdlib] Format and clarify documentation in Unicode.swift

2ec5710

PatrickPijnappel added 2 commits March 14, 2016 09:23

[stdlib] Fix incorrect merge 4d05dc6

549f7e2

[stdlib] Fix doc comment for UnicodeScalar.encode()

43bca07

rintaro reviewed Mar 14, 2016
View reviewed changes

dabrahams reviewed Mar 16, 2016
View reviewed changes

PatrickPijnappel added 3 commits March 17, 2016 05:50

[stdlib] Replace usages of generator with iterator

4d3d723

[stdlib] Use explicit internal in Unicode.swift

7e0945e

[stdlib] Fix indentation in Unicode.swift

771a815

PatrickPijnappel force-pushed the utf8-rewrite branch from 7707f66 to 771a815 Compare March 16, 2016 19:42

[stdlib] Clarify buffer comments in UTF8._decodeOne()

ffe8646

gribozavr merged commit 168e7e4 into swiftlang:master Mar 22, 2016

PatrickPijnappel deleted the utf8-rewrite branch March 22, 2016 22:55

[stdlib] Rewrite UTF8 decoding #1525

[stdlib] Rewrite UTF8 decoding #1525

Uh oh!

Conversation

PatrickPijnappel commented Mar 3, 2016

What's in this pull request?

Uh oh!

dabrahams commented Mar 8, 2016

Uh oh!

dabrahams Mar 8, 2016

Choose a reason for hiding this comment

Uh oh!

PatrickPijnappel commented Mar 13, 2016

Uh oh!

dabrahams commented Mar 13, 2016

Uh oh!

PatrickPijnappel commented Mar 14, 2016

Uh oh!

gottesmm commented Mar 14, 2016

Uh oh!

rintaro Mar 14, 2016

Choose a reason for hiding this comment

Uh oh!

PatrickPijnappel Mar 14, 2016

Choose a reason for hiding this comment

Uh oh!

rintaro Mar 14, 2016

Choose a reason for hiding this comment

Uh oh!

PatrickPijnappel commented Mar 14, 2016

Uh oh!

dabrahams commented Mar 15, 2016

Uh oh!

dabrahams commented Mar 15, 2016

Uh oh!

PatrickPijnappel commented Mar 16, 2016

Uh oh!

dabrahams commented Mar 16, 2016

Uh oh!

dabrahams Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

PatrickPijnappel Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

dabrahams Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

dabrahams commented Mar 16, 2016

Uh oh!

PatrickPijnappel commented Mar 16, 2016

Uh oh!

gribozavr commented Mar 22, 2016

Uh oh!

Uh oh!