Implement trailing Ogham Space Mark removal per CSS Text 3 [chromium/src : main]
Nils Enevoldsen (Gerrit)
Nils Enevoldsen added 1 comment![]( "Open in Gerrit")
I am a first time contributor so please forgive any inevitable process errors.
Related details
- Javier Fernandez
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
MappingBuilder>::RemoveTrailingOghamSpaceIfExists() {Can you explain why this is a separate function, instead of doing this in `RemoveTrailingCollapsibleSpace`?
Related details
- Javier Fernandez
- Kent Tamura
- Nils Enevoldsen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nils Enevoldsen (Gerrit)
Nils Enevoldsen added 1 comment![]( "Open in Gerrit")
MappingBuilder>::RemoveTrailingOghamSpaceIfExists() {Can you explain why this is a separate function, instead of doing this in `RemoveTrailingCollapsibleSpace`?
I considered both options, but I did it this way because I thought it aligned with the spec more closely. "A sequence of collapsible spaces at the end of a line is removed, as well as any trailing U+1680 OGHAM SPACE MARK whose white-space property is normal, nowrap, or pre-line." (https://drafts.csswg.org/css-text/#white-space-phase-2) The Ogham Space Mark is not collapsible space, it's just handled at the same time. I can move it into `RemoveTrailingCollapsibleSpace` if you prefer. I also considered renaming `RemoveTrailingCollapsibleSpace` to something that would conceptually include the Ogham Space Mark.
Related details
- Javier Fernandez
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nils Enevoldsen (Gerrit)
Nils Enevoldsen added 1 comment![]( "Open in Gerrit")
MappingBuilder>::RemoveTrailingOghamSpaceIfExists() {Nils EnevoldsenCan you explain why this is a separate function, instead of doing this in `RemoveTrailingCollapsibleSpace`?
I considered both options, but I did it this way because I thought it aligned with the spec more closely. "A sequence of collapsible spaces at the end of a line is removed, as well as any trailing U+1680 OGHAM SPACE MARK whose white-space property is normal, nowrap, or pre-line." (https://drafts.csswg.org/css-text/#white-space-phase-2) The Ogham Space Mark is not collapsible space, it's just handled at the same time. I can move it into `RemoveTrailingCollapsibleSpace` if you prefer. I also considered renaming `RemoveTrailingCollapsibleSpace` to something that would conceptually include the Ogham Space Mark.
Correction: I considered doing this in `RemoveTrailingCollapsibleSpaceIfExists`, not in `RemoveTrailingCollapsibleSpace`. Not sure if that's what you meant as well.
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
MappingBuilder>::RemoveTrailingOghamSpaceIfExists() {Nils EnevoldsenCan you explain why this is a separate function, instead of doing this in `RemoveTrailingCollapsibleSpace`?
Nils EnevoldsenI considered both options, but I did it this way because I thought it aligned with the spec more closely. "A sequence of collapsible spaces at the end of a line is removed, as well as any trailing U+1680 OGHAM SPACE MARK whose white-space property is normal, nowrap, or pre-line." (https://drafts.csswg.org/css-text/#white-space-phase-2) The Ogham Space Mark is not collapsible space, it's just handled at the same time. I can move it into `RemoveTrailingCollapsibleSpace` if you prefer. I also considered renaming `RemoveTrailingCollapsibleSpace` to something that would conceptually include the Ogham Space Mark.
Correction: I considered doing this in `RemoveTrailingCollapsibleSpaceIfExists`, not in `RemoveTrailingCollapsibleSpace`. Not sure if that's what you meant as well.
Yeah, it'd be better to do it together. to avoid having a copy of logic to find the last characters, both from code maintenance point of view and runtime performance point of view.
I'm fine to refactor existing functions if doing so helps writing clearer code. If the refactoring goes large, maybe you want to refactor first without changing behaviors though, and you may want a runtime flag to follow the [guideline](https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md).
Related details
- Javier Fernandez
- Kent Tamura
- Nils Enevoldsen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
MappingBuilder>::RemoveTrailingOghamSpaceIfExists() {Nils EnevoldsenCan you explain why this is a separate function, instead of doing this in `RemoveTrailingCollapsibleSpace`?
Nils EnevoldsenI considered both options, but I did it this way because I thought it aligned with the spec more closely. "A sequence of collapsible spaces at the end of a line is removed, as well as any trailing U+1680 OGHAM SPACE MARK whose white-space property is normal, nowrap, or pre-line." (https://drafts.csswg.org/css-text/#white-space-phase-2) The Ogham Space Mark is not collapsible space, it's just handled at the same time. I can move it into `RemoveTrailingCollapsibleSpace` if you prefer. I also considered renaming `RemoveTrailingCollapsibleSpace` to something that would conceptually include the Ogham Space Mark.
Koji IshiiCorrection: I considered doing this in `RemoveTrailingCollapsibleSpaceIfExists`, not in `RemoveTrailingCollapsibleSpace`. Not sure if that's what you meant as well.
Yeah, it'd be better to do it together. to avoid having a copy of logic to find the last characters, both from code maintenance point of view and runtime performance point of view.
I'm fine to refactor existing functions if doing so helps writing clearer code. If the refactoring goes large, maybe you want to refactor first without changing behaviors though, and you may want a runtime flag to follow the [guideline](https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md).
(This code is quite hot that it's good to consider performance)
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job linux-perf/blink_perf.layout failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12c26834090000
Nils Enevoldsen (Gerrit)
Nils Enevoldsen added 1 comment![]( "Open in Gerrit")
MappingBuilder>::RemoveTrailingOghamSpaceIfExists() {Nils EnevoldsenCan you explain why this is a separate function, instead of doing this in `RemoveTrailingCollapsibleSpace`?
Nils EnevoldsenI considered both options, but I did it this way because I thought it aligned with the spec more closely. "A sequence of collapsible spaces at the end of a line is removed, as well as any trailing U+1680 OGHAM SPACE MARK whose white-space property is normal, nowrap, or pre-line." (https://drafts.csswg.org/css-text/#white-space-phase-2) The Ogham Space Mark is not collapsible space, it's just handled at the same time. I can move it into `RemoveTrailingCollapsibleSpace` if you prefer. I also considered renaming `RemoveTrailingCollapsibleSpace` to something that would conceptually include the Ogham Space Mark.
Koji IshiiCorrection: I considered doing this in `RemoveTrailingCollapsibleSpaceIfExists`, not in `RemoveTrailingCollapsibleSpace`. Not sure if that's what you meant as well.
Koji IshiiYeah, it'd be better to do it together. to avoid having a copy of logic to find the last characters, both from code maintenance point of view and runtime performance point of view.
I'm fine to refactor existing functions if doing so helps writing clearer code. If the refactoring goes large, maybe you want to refactor first without changing behaviors though, and you may want a runtime flag to follow the [guideline](https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md).
(This code is quite hot that it's good to consider performance)
Thanks, Koji. I rarely have to worry about performance, but with your feedback I made two main changes in an attempt to improve performance.
First, instead of creating RemoveTrailingOghamSpaceIfExists that parallels RemoveTrailingCollapsibleSpaceIfExists, I made RemoveTrailingOghamSpace that parallels RemoveTrailingCollapsibleSpace. Both functions are called by RemoveTrailingCollapsibleSpaceIfExists, which consequently now has a very slightly inaccurate name.
Second, I hoisted some conditions for RemoveTrailingOghamSpace into RemoveTrailingCollapsibleSpaceIfExists, and added a condition for has_non_orc_16bit_ that I believe should quickly fail for most non-CJK text on the internet.
I tried benchmarking the impact on this change in two ways.
First, I tried adding a test in third_party/blink/perf_tests/layout/ that created thousands of words with trailing space inside a div, and repeatedly changed the width of the div, running PerfTestRunner.forceLayout each time. The results were too noisy to see any impact of this patch.
Second, I tried adding an extremely targeted microbenchmark in third_party/blink/renderer/core/layout/inline/inline_items_builder_test.cc that specifically called RemoveTrailingCollapsibleSpaceIfExists five million times for four different strings: "hello", "hello ", "漢字テスト", and "漢字テスト ". These results were much less noisy.
Latin, no trailing space: 7 ns baseline, 8 ns with patch
Latin, trailing space: 7 ns baseline, 8 ns with patch
CJK, no trailing space: 7 ns baseline, 10 ns with patch
CJK, trailing space: 7 ns baseline, 10 ns with patch
Then I hooked up a counter to see how often RemoveTrailingCollapsibleSpaceIfExists was called on some typical pages, and tested it on en.wikipedia.org/wiki/Platypus, nytimes.com, and news.cn.
Wikipedia EN article: ~700 calls, ~200 with has_non_orc_16bit_, net ~1.1 µs overhead
New York Times: ~3,700 calls, ~200 with has_non_orc_16bit_, net ~4.3 µs overhead
Xinhua: ~1,800 calls, ~1,000 with has_non_orc_16bit_, net ~4.9 µs overhead
Related details
- Javier Fernandez
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Thanks for the thorough benchmarking, Nils, the numbers look good to me, that said koji is the expert here.
One follow-up on earlier suggestion: since this changes observable behavior on a hot path, it would be worth landing this behind a runtime flag so it can be rolled out safely and disabled quickly if any regression surfaces. The guidelines are here: https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md
A `base::Feature` (e.g. `kTrailingOghamSpaceRemoval`) wired through `RuntimeEnabledFeatures` and checked in `RemoveTrailingCollapsibleSpaceIfExists` before the Ogham branch should be enough i guess
Related details
- Javier Fernandez
- Kent Tamura
- Koji Ishii
- Nils Enevoldsen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nils Enevoldsen (Gerrit)
Nils Enevoldsen added 1 comment![]( "Open in Gerrit")
- Helmut Januschka
- Javier Fernandez
- Kent Tamura
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |