Repository navigation
Conversation
…ationElements::new. r=hsivonen
rustc lowers the `['\u{FFFF}'; 2]` array repeat expression in
`CollationElements::new` to a two-iteration store loop, which LLVM ends
up lowering to a `memset_pattern16` libcall, so every call ends up
calling into libSystem in order to initialise 8 bytes of stack.
Writing the array out element by element gives a plain 8-byte immediate
store instead.
The exact conditions that trigger LLVM's behavior are still under
investigation.
Differential Revision: https://phabricator.services.mozilla.com/D319664
| #[cfg(feature = "latin1")] | ||
| fn split_prefix_latin1<'a, 'b>(left: &'a [u8], right: &'b [u8]) -> (&'a [u8], &'a [u8], &'b [u8]) { | ||
| if cfg!(feature = "serde") { | ||
| // The identical prefix optimization relies on correct data. Turn it off when `serde` is enabled. |
There was a problem hiding this comment.
issue: you can always load data with the _unstable constructor. I do not think this can work.
I don't recall offering this. Maybe I offered something else? I don't understand what is going on here well enough to do this, but I might be misunderstanding your ask. I think it would be helpful to split this PR up as much as possible. There's a lot going on here.
cc @sffc @robertbastian . we were having problems with this when we did it for datetime stuff |
I don't understand this. What's wrong with GIGO here? It is an explicit ICU4X data policy that bad data must never cause unsoundness but is allowed to cause nonsense behavior. I would rather have GIGO than have the optimization not work when serde is enabled. |
|
I told an agent to look for some specific things (specifically around data policy, semver, and unstable) and do a first pass review, here is what it had to say: It does seem to have found some semver issues beyond data policy. Details1. What the PR is Doing (High-Level Overview)This PR is a large mega-PR (
(Note on Henri's PR comment 2. Data Policies, Versioning, and Robustness IssuesThis is the area with the most significant architectural issues relative to ICU4X conventions. A. Misuse of
|
The PR here has but nothing in the PR constructs the
That's #8677. It shows up here only because that PR hasn't landed, yet.
We previously discussed this at length in a meeting an agreed on this approach: This improves performance when
The issue is that the comparison functions exhibiting GIGO in a way that doesn't implement a total order has worse outcomes than GIGO usually has. With Rust With FFI and the C++ standard library collections GIGO leading to comparison not implementing total order leads to memory-unsafety. I suppose we could just do GIGO anyway and document the potential risks.
Listed as open issue.
The PR tries to use the
I had forgotten about this. I have already forgotten what perf measurements I did here.
We've never generated data that could be rejected, and we've never suggested customizability of normalizations in a way that would be this large a departure from how the specced normalizations behave for characters that have been assigned forever.
Listed as open issue.
I benchmarked this a lot, and to get proper perf out of this, it made sense to do SIMD expansion first intead of having this method do one-by-one ALU expansion. Needs docs, sure. (Notably, doing SIMD expansion on a per-subslice basis did not perform well enough. IIRC, I even tried to write more than logically necessary in order not to fall back to ALU when the subslice length wasn't a multiple of the SIMD stride.)
Unicode stability policies mean that how the Latin1 range normalizes never changes, so dynamic data loading does not make sense. |
Ah. Yes, I can help with that but the PR really needs to be split into much smaller ones if I am to have a hope of understanding things sufficiently. I also do not know if I currently will have enough time for this, that offer was made quite a while ago with the expectation that this was coming in a few months.
Hmm, interesting point, we should probably discuss this as a WG. I am inclined towards using GIGO anyway and documenting. One thought is that we can keep this as a separate "serde_total_order_guarantee" feature where you can opt in to that guarantee for serde mode.
Since the last time we have discussed this we have realized that this type of hack is not really very pleasant and causes problems: #8576 So I'm a bit wary. I'd also like to have serde users get this optimization if they'd like. |
|
Path forward:
|
|
My understanding from discussion with @hsivonen: There are multiple invariants that the collator data structs need to uphold in order for them to produce a total ordering. Here are two that Henri explained:
The contractions invariant landed in d01e47e, which I verified is in 2.0, which means that the V1 struct has that invariant. When we create a V2, it will add the additional invariant. If we get V1 data in the Serde constructor, we can validate it to make it V2 data. |
|
Thought after the meeting: Upgrading bogus V2 to V2 is the same operation as upgrading V1 to V2, so if we validate dynamically-loaded V2, we might as well fix it up instead of rejecting. If we do that, there's no difference between loading V1 and V2. |
|
The upgrade function is |
Ah, but fixing it up is expensive, right? Is it cheap to detect? We're generally fine with "serde with V1 is slow but becomes fast if you load V2" (fast = normal performance, does not need to achieve all optimizations). We're less fine with pessimizing serde data load significantly with the current version of data. My understanding was that the route was to have V2 be explicitly defined as having a particular invariant (that old generated data does not have). The changed marker is the signal that the data has this invariant, and we don't actually check it. If the V1 struct already supports a cheaply-checked way to detect that the data was intended to have the reordering invariant, then yeah we don't need to have a V2. But if it requires expensive validation to detect that, we shouldn't. |
|
Validation is expensive (iterate though collation trie's data array and do a normalization trie lookup for most of those entries). Fix-up is validation, allocating from ZeroSlice to ZeroVec, and a few bitops. |
|
@hsivonen okay, in that case we should always make sure that serde users have a way to avoid expensive iterations by generating new data. This can either mean that we generate V1 data with some bit set that indicates that it is "newer", or we generate V2 data and use the data marker as a disambiguator. Defaulting to assuming the latter is easier. |
|
OK. |
|
I thought about the data struct upgrade some more. With real data, I expect rewriting the data array to be possible but theoretically wrong. It's both easier and theoretically right to use the new builder code instead. If we are OK with running the builder code, we could give |
|
Yes, I am okay with running the builder code in cases where serde users are loading old data (not new data) |
|
#8694 now runs the builder for NFD/NFKD. |
Changelog
icu_collator
serdefeature is enabled (to ensure that the comparison functions implement a total order even with bogus data)CodePointTries in collation data. (datagen has never provided an option to generate different tries.)CodePointTrielookups with UTF decoding.serdefeature is not used, since this requires normalization and collation data to agree for the comparison function to implement a total order and is also not compatible with previously-generated data).icu_normalizer
CodePointTrieare fused with UTF decoding.CodePointTrie-based data structure for the canonical composition operation.Open issues
CollationRootV1andCollationTailoringV1are not incremented for the new kind of quick primary check data, because the new kind of data isn't used ifserdeis enabled and when the new quick primary check code is not in use, old data works. (If we want this optimization to work withserde, we'd have to do heavier validation of data: iterate over the trie data and cross-check with the normalization trie that the values are consistent.)serde-enabled builds lose the identical prefix optimization but not because of data changes here: It was already bad to have it with GIGO. It we wanted to validate data for correctness on this point, we'd need to be able to makeChar16Trietell us what strings it can match. I don't know if that's possible. Specifically, the validation would have to find all contractions that match 3 or more characters and end with a starter. Then it would have to check that the characters in the middle (non-first, non-last) of these strings also have contraction CE32s.Closes #7526