Modify patterns from a skeleton match to have the correct width #832
Conversation
Pull Request Test Coverage Report for Build a51364a4f74669190872dd6e0ea8e3bd91dac4bb-PR-832
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## main #832 +/- ##
==========================================
+ Coverage 74.80% 74.86% +0.06%
==========================================
Files 198 198
Lines 12762 12778 +16
==========================================
+ Hits 9546 9566 +20
+ Misses 3216 3212 -4
Continue to review full report at Codecov.
|
…skeleton match to have the correct width"
…skeleton match to have the correct width"
| // There's no match, or this is a string literal return the original item. | ||
| item.clone() | ||
| }) | ||
| .collect::<Vec<PatternItem>>(), |
There was a problem hiding this comment.
Thought: I wish we could do this without alloc. I liked how before this function was basically a projection from &'a SkeletonsV1 to &'a Pattern. The extra allocation is going to hurt basically every benchmark we're after: memory usage, code size, and performance.
Suggestion: Could this be architected in a way that involves a map on the pattern itself? Like, you could attach a mapping function to the pattern, such that when you loop over the pattern while formatting, you run the PatternItems through the mapping function.
Alternatively, is this in the part of the code that we won't use at runtime after the new CLDR 70 skeleton algorithm is implemented? I don't care about the Vec if it's run in the transformer.
There was a problem hiding this comment.
The extra allocation is going to hurt basically every benchmark we're after.
Yes, I agree that the allocation isn't great, and what I tried to design around in the initial implementation.
Suggestion: Could this be architected in a way that involves a map on the pattern itself.
The pattern must be mutated in order to support all of the features for DateTimeFormat. Beyond just the widths, there's also the hour cycle to consider. There's also append items support for things like time zones or week. The latter is even trickier, as a simple mapping function wouldn't work.
I would prefer to accept the perf/memory regression for 0.3, and follow-up with exploring some approaches. Especially as pattern mutation is a new requirement for other features such as the hour cycle, and append items. Then for 0.4 I can track fixing it.
Alternatively, is this in the part of the code that we won't use at runtime after the new CLDR 70 skeleton algorithm is implemented? I don't care about the Vec if it's run in the transformer.
I don't think well want a literal pattern for every width adjustment, as it will greatly affect the provider data. I'd say let's follow-up with looking for perf wins here. I'm nervous about the trade off between runtime characteristics and data payload size. I think it's something we should definitely try and work out.
There was a problem hiding this comment.
I filed #877 as a follow-up, if you are OK with moving forward with this patch.
There was a problem hiding this comment.
Okay. I won't block this PR but I'm a bit disappointed that we're going in this direction. I feel like we're repeating the problems of ICU4C rather than solving them. I hope we can follow up in #877.
There was a problem hiding this comment.
Well, this at least gets us a baseline metric that we can track, and tests in place that make sure we are compliant with the behavior. The code is mutable, so we can course correct to fewer allocations. I also filed #879 to make sure we track this area better. I realize now that the benchmark is still needed.
Resolves #584.