[stdlib] Unicode 9 and Thread Local Storage#9265
Merged
milseman merged 9 commits intoswiftlang:masterfrom May 11, 2017
Merged
Conversation
Member
Author
|
@swift-ci please test Not just for kicks, I'm genuinely interested |
milseman
commented
May 4, 2017
Member
Author
There was a problem hiding this comment.
As @compnerd mentioned, we can remove the "-l" part
Member
|
@swift-ci please test |
Member
|
@swift-ci please smoke benchmark |
Contributor
|
Build failed |
Contributor
|
Build failed |
Contributor
Build comment file:Optimized (O) Regression (24)
Improvement (10)
No Changes (235)
Regression (15)
Improvement (7)
No Changes (247)
|
Member
|
I think those benchmarks are showing changes on master from other things that went in since, like the new utf8 decoding and the change to remove <, not this change. Needs rebasing and rebenchmarking. |
Member
Author
|
@swift-ci please test |
Contributor
|
Build failed |
Contributor
|
Build failed |
Member
Author
|
Rewrote history a little to squash some things, and only pull in the tiny bits of ICU that we use. |
Member
Author
|
Added in more static protections for cross-platform differences and a cache coherency fix. |
Member
Author
|
@swift-ci please smoke test Linux platform |
Member
Author
|
@swift-ci please smoke benchmark |
Member
Author
|
I extracted some of those smoke benchmarks, and I'm seeing us being way faster with this PR compared to a couple weeks ago (before a lot of my other optimizations). I'm measuring that we are: than a couple weeks ago. |
Member
Author
looks like the builder's broken, as other PRs are hitting this. Will try again soon. |
Member
Author
|
@swift-ci please smoke test Linux platform |
Contributor
|
Just for bookkeeping: This supersedes #3125, right? |
Member
Author
|
@CodaFi when it lands, yes |
Contributor
Build comment file:Optimized (O) Regression (2)
Improvement (2)
No Changes (265)
Regression (2)
Improvement (1)
No Changes (266)
|
Member
Author
|
Sweet, that looks better. I'll look into those. |
Member
Author
|
Linux platform defines a different pthread_key_t, as suspected. Will fix. |
Member
Author
|
@swift-ci please smoke test Linux Platform |
Member
Author
|
SuperChars is a legit slowdown. Grapheme breaking performance is sometimes faster and sometimes slower than the tries if we can't hit our fast paths. Some of the delta is the same inefficient Character construction from a (presumably) valid CharcterView.Index, but not most. @airspeedswift Any ideas? Are you ok with this? |
Member
|
I think the regression in SuperChars is fine, that benchmark isn't very representative of real usage. |
Member
Author
|
@swift-ci please smoke test Linux Platform |
Member
Author
|
@swift-ci please test OS X Platform |
Contributor
|
Build failed |
Member
Author
|
@swift-ci please smoke test Linux Platform |
1 similar comment
Member
Author
|
@swift-ci please smoke test Linux Platform |
Introduce shims for using UBreakIterators from ICU. Also introduce shims for using thread local storage via pthreads. We will be relying on ICU and UBreakIterators for grapheme breaking. But, UBreakIterators are very expensive to create, especially for the way we do grapheme breaking, which is relatively stateless. Thus, we will stash one or more into thread local storage and reset it as needed. Note: Currently, pthread_key_t is hard coded for a single platform (Darwin), but I have a static_assert alongside directions on how to adapt it to any future platforms who differ in key type.
Introduces a _ThreadLocalStorage struct to hold thread-local, but global resources. Set it up to host a UBreakIterator and a cache key for resetting text. UBreakIterators are extremely expensive to create on the fly, so we store one for each thread. UBreakIterators are also expensive to bind to new text, so we cache the text it's currently bound to in order to help avoid it. The struct can be expanded with more functionality in the future, but the standard library should only ever use a single key, and thus everything should go on this struct. The _ThreadLocalStorage struct is not meant to be copyable, creatable (by anyone else except the once-per-thread initialize routine), and should accessed through the pointers it provides. Future immediate directions could include cashing multiple UBreakIterators (e.g. avoid a text reset for mutual character iteration patterns, etc). Test added in test/stdlib/ThreadLocalStorage.swift.
Use UBreakIterators to perform grapheme breaking. This gives Unicode 9 grapheme breaking (e.g. family emoji) and provides a means to upgrade to future versions. It also serves as a model for how to serve up other advanced functionality in ICU to users. This has tricky performance implications. Some things are faster and a number of cases are slower. But, careful use of ICU can help mitigate and amortize these costs. In conjunction with more early detection of fast paths, overall grapheme breaking for the average user should be much faster than in Swift 3. NOTE: This is incomplete. It currently falls back on the legacy tries for some bridged strings. There are many potential directions for a general solution, but for now we'll be interatively adding support for more and more special cases.
Adds in Linux platform support for our pthread TLS. Replace usage of PTHREAD_KEYS_MAX with a sentinel value, as it's tricky to define cross-platform and was only lightly used inside sanity checks.
Member
Author
|
@swift-ci please smoke test |
Disables the portions of the regional break test file corresponding to Unicode 8.0 vs 9.0 differences. Once the compiler and stdlib are in sync with Unicode 9.0, and the legacy tries are dropped, we should splat down newer versions of these test files.
Member
Author
|
@swift-ci please test |
Contributor
|
Build failed |
Contributor
|
Build failed |
Member
Author
|
Just to be extra cautious, I'm going to wait for that OS X test to finish... |
shahmishal
added a commit
that referenced
this pull request
May 11, 2017
Revert "Merge pull request #9265 from milseman/tls_ftw"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR Add support for Unicode 9 grapheme breaking (i.e. emoji) by using ICU. Work around ICU efficiency issues with thread-local storage.
Introduce shims for using UBreakIterators from ICU. Also introduce
shims for using thread local storage via pthreads.
We will be relying on ICU and UBreakIterators for grapheme
breaking. But, UBreakIterators are very expensive to create,
especially for the way we do grapheme breaking, which is relatively
stateless. Thus, we will stash one or more into thread local storage
and reset it as needed.
Introduces a _ThreadLocalStorage struct to hold thread-local, but
global resources. Set it up to host a UBreakIterator and a cache key
for resetting text.
UBreakIterators are extremely expensive to create on the fly, so we
store one for each thread. UBreakIterators are also expensive to bind
to new text, so we cache the text it's currently bound to in order to
help avoid it.
The struct can be expanded with more functionality in the future, but
the standard library should only ever use a single key, and thus
everything should go on this struct. The _ThreadLocalStorage struct is
not meant to be copyable, creatable (by anyone else except the
once-per-thread initialize routine), and should accessed through the
pointers it provides.
Future immediate directions could include cashing multiple
UBreakIterators (e.g. avoid a text reset for mutual character
iteration patterns, etc).
Test added in test/stdlib/ThreadLocalStorage.swift.
Use UBreakIterators to perform grapheme breaking. This gives Unicode 9
grapheme breaking (e.g. family emoji) and provides a means to upgrade
to future versions. It also serves as a model for how to serve up
other advanced functionality in ICU to users.
This has tricky performance implications. Some things are faster and a
number of cases are slower. But, careful use of ICU can help mitigate
and amortize these costs. In conjunction with more early detection of
fast paths, overall grapheme breaking for the average user should be
much faster than in Swift 3.
NOTE: This is incomplete. It currently falls back on the legacy tries
for some bridged strings. There are many potential directions for a
general solution, but for now we'll be interatively adding support for
more and more special cases.