Conversation
|
If you want to modify |
Cased tableCased Unicode table
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Remove `Cased` Unicode table
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (abd6680): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 466.287s -> 464.864s (-0.31%) |
|
☔ The latest upstream changes (presumably #146173) made this pull request unmergeable. Please resolve the merge conflicts. |
a765086 to
4ca4c44
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #147340) made this pull request unmergeable. Please resolve the merge conflicts. |
4ca4c44 to
94be7eb
Compare
This comment has been minimized.
This comment has been minimized.
|
@scottmcm ping? |
|
☔ The latest upstream changes (presumably #148337) made this pull request unmergeable. Please resolve the merge conflicts. |
94be7eb to
55adb69
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #148436) made this pull request unmergeable. Please resolve the merge conflicts. |
`Cased` is a derived property - it is the union of the `Lowercase` property, the `Uppercase` property, and the `Titlecase_Letter` general categories. We already have lookup tables for `Lowercase` and `Uppercase`, and `Titlecase_Letter` is very small. So instead of duplicating a lookup table for `Cased`, just test each of those properties in turn. This probably will be slower than the old approach, but it is not a public API: it is only used in `string::to_lower` when deciding when a Greek "sigma" should be mapped to `ς` or to `σ`. This is a very rare case, so should not be performance sensitive.
55adb69 to
a9b456f
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment was marked as resolved.
This comment was marked as resolved.
|
I'm surprised that this appears to increase binary size?! If that's still the case, then I don't think we should do this. Otherwise the argument about only being used in @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Remove `Cased` Unicode table
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (97f47c0): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.6%, secondary -1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.1%, secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 473.018s -> 473.56s (0.11%) |
|
Actually it makes sense that this yields a mixed bag of results: For code that only calls Given that the |
If I understand correctly, what you're saying is Before this PR, After this PR, Calls to So this PR is a win for programs that contain calls to Is that right? If so, I agree with your reasoning. |
Split off from #145219
Casedis a derived property - it is the union of theLowercaseproperty, theUppercaseproperty, and theTitlecase_Lettergeneral categories. We already have lookup tables forLowercaseandUppercase, andTitlecase_Letteris very small. So instead of duplicating a lookup table forCased, just test each of those properties in turn.This probably will be slower than the old approach, but it is not a public API: it is only used in
string::to_lowerwhen deciding when a Greek "sigma" should be mapped toςor toσ. This is a very rare case, so should not be performance sensitive.