Speed up ScriptRunIterator. [chromium/src : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1595e2f0c90000
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10904294c90000
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/103a3748c90000
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 3 comments![]( "Open in Gerrit")
again. Each character incurs overhead from calling ICU, resizing
vectors, intersecting and so on.Resizing vectors in this case are the vectors of possible scripts per character? In a separate change, or in this one, can we just keep these vectors pre-allocated or use a fixed maximum size std::array for them?
Add a fast path so that once we're down at a singleton script,
we consult a bitmap (lazily built, kept per-process) of which
code points are allowed in that script. This allows us to moveCan we potentially build this at compile time? I don't fully understand the definition of this map/bitmap yet. Can you define in Unicode terms what is in there? Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?
Can you define "Skippable" in Unicode terms?
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>I would prefer if we could move this to `character.h`/`character.cc` or into something suitable in `wtf/text` and describe what it returns in Unicode terms, then use it from here. Reading the code below, it seems computable at compile time? Can the bitset be initialized with a constant?
When moving this to character/text - as the API, can we separate items returned in the pair? Is the second return value, `inherited_not_common` actually dependent on the input script?
Related details
- Steinar H Gunderson
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 2 comments![]( "Open in Gerrit")
again. Each character incurs overhead from calling ICU, resizing
vectors, intersecting and so on.Resizing vectors in this case are the vectors of possible scripts per character? In a separate change, or in this one, can we just keep these vectors pre-allocated or use a fixed maximum size std::array for them?
They are already pre-allocated to the best of our ability; we don't call malloc. But they cannot be fixed-size as long as we want to do stuff like push_front().
Add a fast path so that once we're down at a singleton script,
we consult a bitmap (lazily built, kept per-process) of which
code points are allowed in that script. This allows us to moveCan we potentially build this at compile time? I don't fully understand the definition of this map/bitmap yet. Can you define in Unicode terms what is in there? Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?
Can you define "Skippable" in Unicode terms?
We can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Can you say what you are looking for wrt. “Unicode terms”? I don't know what terms you are looking for beyond scripts and script extensions. :-) It is mostly defined in terms of “does GetScripts() contain the script”, which is already poorly documented, but you could say something like “is the given script the primary script for the character, or in the character's list of script extensions”. Except that common and inherited have their own twists on top of that, which I have to emulate. The functional explanation is “if we know the previous run is in a given script, do we know for sure that this character can extend that run (and is not a bracket)”.
Related details
- Dominik Röttsches
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. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 7 comments![]( "Open in Gerrit")
Add a fast path so that once we're down at a singleton script,
we consult a bitmap (lazily built, kept per-process) of which
code points are allowed in that script. This allows us to moveSteinar H GundersonCan we potentially build this at compile time? I don't fully understand the definition of this map/bitmap yet. Can you define in Unicode terms what is in there? Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?
Can you define "Skippable" in Unicode terms?
We can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Can you say what you are looking for wrt. “Unicode terms”? I don't know what terms you are looking for beyond scripts and script extensions. :-) It is mostly defined in terms of “does GetScripts() contain the script”, which is already poorly documented, but you could say something like “is the given script the primary script for the character, or in the character's list of script extensions”. Except that common and inherited have their own twists on top of that, which I have to emulate. The functional explanation is “if we know the previous run is in a given script, do we know for sure that this character can extend that run (and is not a bracket)”.
We can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Perhaps compile time compute for the most used scripts? Arab, Cyrl, Hani, Jpan, Kore, Latn (in alphabetic order)? But that can be a follow up.
Can you say what you are looking for wrt. “Unicode terms”?
Yes, in my feedback, I tried to draft such a description: "Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?" - And it sounds like that description is similar to what you're saying “is the given script the primary script for the character, or in the character's list of script extensions”.
My intention was to generally improve naming and clarity her and work towards a name more descriptive that "GetSkippable", by finding a more Unicode-oriented way to describe the function return values.
Editor-TipTap [ -5.8%, -5.5%]Does this mostly affect the Latin parts of Editor-TipTap? Or does this optimization hit the CJK part of Editor-TipTap, too? Or is Editor TipTap only the French Guttenberg text and the long programming code by now? I remember it had a CJK text, or did it?
virtual std::pair<const ScriptData::UnicodeBitSet*,
const ScriptData::UnicodeBitSet*>The return values are only discenible by reading the comment. I suggest to make this a struct, or preferred also compute `inherited_not_common_chars_` if it is really independent of `UScriptCode script` input argument.
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>I would prefer if we could move this to `character.h`/`character.cc` or into something suitable in `wtf/text` and describe what it returns in Unicode terms, then use it from here. Reading the code below, it seems computable at compile time? Can the bitset be initialized with a constant?
When moving this to character/text - as the API, can we separate items returned in the pair? Is the second return value, `inherited_not_common` actually dependent on the input script?
Thinking about this more, as the behavior is quite tightly coupled with this implementation, I'd say moving this to `wft/text` is optional. On the other hand, if we generate parts of the bitsets at compile time and would need a generator executable (like other character properties) then we better do that in the same platform/text/ where we also have `character_property_data_generator.cc`.
ALWAYS_INLINE static bool IsSet(Suggest to improve name, perhaps `MatchesScript`, `DoesNotIntroduceScriptBreak`, or something like that?
// If we're down to a singleton script, anything that's allowed in thatCan this be moved into a helper function that is labelled as the optimized forward scan?
// SAFETY: We already tested that ahead_pos_ <= length_ above, and
// ahead_pos_ is unsigned, so we know that ptr is within text_[0..length]
// at all times, and within text_[0..length) when we dereference it due to
// the check (ptr != end) in the for loop.I think this SAFETY comment should discuss that the `++ptr` increment may lead to hitting a surrogate and that `IsSet` handles that?
Related details
- Steinar H Gunderson
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 6 comments![]( "Open in Gerrit")
Add a fast path so that once we're down at a singleton script,
we consult a bitmap (lazily built, kept per-process) of which
code points are allowed in that script. This allows us to moveSteinar H GundersonCan we potentially build this at compile time? I don't fully understand the definition of this map/bitmap yet. Can you define in Unicode terms what is in there? Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?
Can you define "Skippable" in Unicode terms?
Dominik RöttschesWe can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Can you say what you are looking for wrt. “Unicode terms”? I don't know what terms you are looking for beyond scripts and script extensions. :-) It is mostly defined in terms of “does GetScripts() contain the script”, which is already poorly documented, but you could say something like “is the given script the primary script for the character, or in the character's list of script extensions”. Except that common and inherited have their own twists on top of that, which I have to emulate. The functional explanation is “if we know the previous run is in a given script, do we know for sure that this character can extend that run (and is not a bracket)”.
We can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Perhaps compile time compute for the most used scripts? Arab, Cyrl, Hani, Jpan, Kore, Latn (in alphabetic order)? But that can be a follow up.
Can you say what you are looking for wrt. “Unicode terms”?
Yes, in my feedback, I tried to draft such a description: "Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?" - And it sounds like that description is similar to what you're saying “is the given script the primary script for the character, or in the character's list of script extensions”.
My intention was to generally improve naming and clarity her and work towards a name more descriptive that "GetSkippable", by finding a more Unicode-oriented way to describe the function return values.
Perhaps compile time compute for the most used scripts? Arab, Cyrl, Hani, Jpan, Kore, Latn (in alphabetic order)? But that can be a follow up.
To be clear, is this because you expect speed improvements from it? I can test that if you wish, although I'm not optimistic it will matter a lot.
Yes, in my feedback, I tried to draft such a description: "Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?"
The problem is that it's not actually the truth; GetScripts() does a lot of fiddling with e.g. inherited characters, and we also have brackets to worry about. “skippable” is meant to be functional (what can the external caller use this for, not how is it built), since it's hard to make a short and accurate name.
I mean, we can't call it GetRunesThatAreNotSurrogatesAndMatchOurPrimaryScriptOrExtensionScript(OrOtherTypesOfHana)AndAlsoCommonOrInheritedCharactersWithoutExtensionScripts() :-)
What about something like GetSafeToExtendExistingRun(UCodeScript script)?
Editor-TipTap [ -5.8%, -5.5%]Does this mostly affect the Latin parts of Editor-TipTap? Or does this optimization hit the CJK part of Editor-TipTap, too? Or is Editor TipTap only the French Guttenberg text and the long programming code by now? I remember it had a CJK text, or did it?
I don't know what goes into Editor-TipTap. On screen, it only shows Latin text, and I can only find that in resources/editors/longtext.html, but I can't guarantee that it doesn't try to shape CJK somewhere off-screen.
I believe the optimization should work well for Chinese (which mainly consists of long strings of Han characters, AFAIK) but less so for Japanese (which frequently mixes kanji and hana), and I don't know enough about hangul in Unicode to say anything about Korean.
virtual std::pair<const ScriptData::UnicodeBitSet*,
const ScriptData::UnicodeBitSet*>The return values are only discenible by reading the comment. I suggest to make this a struct, or preferred also compute `inherited_not_common_chars_` if it is really independent of `UScriptCode script` input argument.
It's independent, but it's made on-the-fly because we don't need it earlier.
The problem of not returning is; how would you ever do that? As long as ScriptData is a pure virtual, we need these kind of gymnastics to return anything. We could have two calls, of course, but I'm not sure if that's an improvement, since every caller ever would call both.
ALWAYS_INLINE static bool IsSet(Suggest to improve name, perhaps `MatchesScript`, `DoesNotIntroduceScriptBreak`, or something like that?
No, that's wrong. Note that it's used for testing inherited_not_common_chars, too.
// If we're down to a singleton script, anything that's allowed in thatCan this be moved into a helper function that is labelled as the optimized forward scan?
I'm not sure if that makes sense; it's intimately tied to local variables in the function. What would the signature look like?
// SAFETY: We already tested that ahead_pos_ <= length_ above, and
// ahead_pos_ is unsigned, so we know that ptr is within text_[0..length]
// at all times, and within text_[0..length) when we dereference it due to
// the check (ptr != end) in the for loop.I think this SAFETY comment should discuss that the `++ptr` increment may lead to hitting a surrogate and that `IsSet` handles that?
No, the point of SAFETY comments is to justify that the UNSAFE_BUFFERS() does not go out of bounds, nothing else should be there. I can add the part of hitting surrogates if you wish, but it does not belong in the SAFETY comment (it comes from the IsSet() test as you say).
Related details
- Dominik Röttsches
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. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 3 comments![]( "Open in Gerrit")
Add a fast path so that once we're down at a singleton script,
we consult a bitmap (lazily built, kept per-process) of which
code points are allowed in that script. This allows us to moveSteinar H GundersonCan we potentially build this at compile time? I don't fully understand the definition of this map/bitmap yet. Can you define in Unicode terms what is in there? Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?
Can you define "Skippable" in Unicode terms?
Dominik RöttschesWe can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Can you say what you are looking for wrt. “Unicode terms”? I don't know what terms you are looking for beyond scripts and script extensions. :-) It is mostly defined in terms of “does GetScripts() contain the script”, which is already poorly documented, but you could say something like “is the given script the primary script for the character, or in the character's list of script extensions”. Except that common and inherited have their own twists on top of that, which I have to emulate. The functional explanation is “if we know the previous run is in a given script, do we know for sure that this character can extend that run (and is not a bracket)”.
Steinar H GundersonWe can build it at compile time, assuming you're fine with shipping 0xD800 * 250 bits = ~1.7 MiB of extra data in the binary. There are a lot of scripts.
Perhaps compile time compute for the most used scripts? Arab, Cyrl, Hani, Jpan, Kore, Latn (in alphabetic order)? But that can be a follow up.
Can you say what you are looking for wrt. “Unicode terms”?
Yes, in my feedback, I tried to draft such a description: "Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?" - And it sounds like that description is similar to what you're saying “is the given script the primary script for the character, or in the character's list of script extensions”.
My intention was to generally improve naming and clarity her and work towards a name more descriptive that "GetSkippable", by finding a more Unicode-oriented way to describe the function return values.
Perhaps compile time compute for the most used scripts? Arab, Cyrl, Hani, Jpan, Kore, Latn (in alphabetic order)? But that can be a follow up.
To be clear, is this because you expect speed improvements from it? I can test that if you wish, although I'm not optimistic it will matter a lot.
Yes, in my feedback, I tried to draft such a description: "Does it describe the intersection of input script argument with the union of primary script and script extensions per codepoint in the bitset?"
The problem is that it's not actually the truth; GetScripts() does a lot of fiddling with e.g. inherited characters, and we also have brackets to worry about. “skippable” is meant to be functional (what can the external caller use this for, not how is it built), since it's hard to make a short and accurate name.
I mean, we can't call it GetRunesThatAreNotSurrogatesAndMatchOurPrimaryScriptOrExtensionScript(OrOtherTypesOfHana)AndAlsoCommonOrInheritedCharactersWithoutExtensionScripts() :-)
What about something like GetSafeToExtendExistingRun(UCodeScript script)?
To be clear, is this because you expect speed improvements from it? I can test that if you wish, although I'm not optimistic it will matter a lot.
I think it's less wasteful if it's reasonably small and compile time doable. We do that for a set of other per-Character properties to move tham to a more digestible form for consumption, when ICU doesn't have the most efficient interface to access them at runime.
GetSafeToExtendExistingRun(UCodeScript script)?
WFM, thanks.
ALWAYS_INLINE static bool IsSet(Steinar H GundersonSuggest to improve name, perhaps `MatchesScript`, `DoesNotIntroduceScriptBreak`, or something like that?
No, that's wrong. Note that it's used for testing inherited_not_common_chars, too.
Acknowledged
// SAFETY: We already tested that ahead_pos_ <= length_ above, and
// ahead_pos_ is unsigned, so we know that ptr is within text_[0..length]
// at all times, and within text_[0..length) when we dereference it due to
// the check (ptr != end) in the for loop.Steinar H GundersonI think this SAFETY comment should discuss that the `++ptr` increment may lead to hitting a surrogate and that `IsSet` handles that?
No, the point of SAFETY comments is to justify that the UNSAFE_BUFFERS() does not go out of bounds, nothing else should be there. I can add the part of hitting surrogates if you wish, but it does not belong in the SAFETY comment (it comes from the IsSet() test as you say).
- Steinar H Gunderson
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 2 comments![]( "Open in Gerrit")
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>I would prefer if we could move this to `character.h`/`character.cc` or into something suitable in `wtf/text` and describe what it returns in Unicode terms, then use it from here. Reading the code below, it seems computable at compile time? Can the bitset be initialized with a constant?
When moving this to character/text - as the API, can we separate items returned in the pair? Is the second return value, `inherited_not_common` actually dependent on the input script?
Thinking about this more, as the behavior is quite tightly coupled with this implementation, I'd say moving this to `wft/text` is optional. On the other hand, if we generate parts of the bitsets at compile time and would need a generator executable (like other character properties) then we better do that in the same platform/text/ where we also have `character_property_data_generator.cc`.
I ran a test where I precompute the bit sets that we use in Speedometer:
https://pinpoint-dot-chromeperf.appspot.com/job/16cec652c90000
There is no win over making it on-the-fly as we do here.
// SAFETY: We already tested that ahead_pos_ <= length_ above, and
// ahead_pos_ is unsigned, so we know that ptr is within text_[0..length]
// at all times, and within text_[0..length) when we dereference it due to
// the check (ptr != end) in the for loop.Steinar H GundersonI think this SAFETY comment should discuss that the `++ptr` increment may lead to hitting a surrogate and that `IsSet` handles that?
Dominik RöttschesNo, the point of SAFETY comments is to justify that the UNSAFE_BUFFERS() does not go out of bounds, nothing else should be there. I can add the part of hitting surrogates if you wish, but it does not belong in the SAFETY comment (it comes from the IsSet() test as you say).
Ok, let's add it outside of `SAFETY:`.
- Dominik Röttsches
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. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 3 comments![]( "Open in Gerrit")
virtual std::pair<const ScriptData::UnicodeBitSet*,
const ScriptData::UnicodeBitSet*>The return values are only discenible by reading the comment. I suggest to make this a struct, or preferred also compute `inherited_not_common_chars_` if it is really independent of `UScriptCode script` input argument.
It's independent, but it's made on-the-fly because we don't need it earlier.
The problem of not returning is; how would you ever do that? As long as ScriptData is a pure virtual, we need these kind of gymnastics to return anything. We could have two calls, of course, but I'm not sure if that's an improvement, since every caller ever would call both.
I suggest at least a custom struct return to give them names. `std::pair` is used here as "two independent things", which is not descriptive in code.
How about defining a type
`struct RunExtensionLookups {
const ScriptData::UnicodeBitSet can_remain_in_script_;
const ScriptData::UnicodeBitSet inherited_not_common_;
}` and using this as the return type.
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>Dominik RöttschesI would prefer if we could move this to `character.h`/`character.cc` or into something suitable in `wtf/text` and describe what it returns in Unicode terms, then use it from here. Reading the code below, it seems computable at compile time? Can the bitset be initialized with a constant?
When moving this to character/text - as the API, can we separate items returned in the pair? Is the second return value, `inherited_not_common` actually dependent on the input script?
Steinar H GundersonThinking about this more, as the behavior is quite tightly coupled with this implementation, I'd say moving this to `wft/text` is optional. On the other hand, if we generate parts of the bitsets at compile time and would need a generator executable (like other character properties) then we better do that in the same platform/text/ where we also have `character_property_data_generator.cc`.
I ran a test where I precompute the bit sets that we use in Speedometer:
https://pinpoint-dot-chromeperf.appspot.com/job/16cec652c90000
There is no win over making it on-the-fly as we do here.
Acknowledged
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>Does this mean the optimization is switched off for testing? I believe we do need correctness coverage for the optimization as well. I am concerned otherwise it's too difficult to maintain correctness and performance for future developers treading here. Having two code paths increases the risk of diverging.
Related details
- Steinar H Gunderson
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 2 comments![]( "Open in Gerrit")
virtual std::pair<const ScriptData::UnicodeBitSet*,
const ScriptData::UnicodeBitSet*>Steinar H GundersonThe return values are only discenible by reading the comment. I suggest to make this a struct, or preferred also compute `inherited_not_common_chars_` if it is really independent of `UScriptCode script` input argument.
Dominik RöttschesIt's independent, but it's made on-the-fly because we don't need it earlier.
The problem of not returning is; how would you ever do that? As long as ScriptData is a pure virtual, we need these kind of gymnastics to return anything. We could have two calls, of course, but I'm not sure if that's an improvement, since every caller ever would call both.
I suggest at least a custom struct return to give them names. `std::pair` is used here as "two independent things", which is not descriptive in code.
How about defining a type
`struct RunExtensionLookups {
const ScriptData::UnicodeBitSet can_remain_in_script_;
const ScriptData::UnicodeBitSet inherited_not_common_;
}` and using this as the return type.
Done
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>Does this mean the optimization is switched off for testing? I believe we do need correctness coverage for the optimization as well. I am concerned otherwise it's too difficult to maintain correctness and performance for future developers treading here. Having two code paths increases the risk of diverging.
It's switched off for MockScriptIterator. It is used for all other tests (both unit tests and WPT tests). Most tests don't use MockScriptIterator.
Related details
- Dominik Röttsches
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. |
Dominik Röttsches (Gerrit)
Dominik Röttsches voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominik Röttsches
| Code-Review | +1 |
2 comments
std::pair<const ScriptData::UnicodeBitSet*, const ScriptData::UnicodeBitSet*>Steinar H GundersonDoes this mean the optimization is switched off for testing? I believe we do need correctness coverage for the optimization as well. I am concerned otherwise it's too difficult to maintain correctness and performance for future developers treading here. Having two code paths increases the risk of diverging.
It's switched off for MockScriptIterator. It is used for all other tests (both unit tests and WPT tests). Most tests don't use MockScriptIterator.
- Steinar H Gunderson
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson voted and added 4 comments![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Commit-Queue | +2 |
4 comments
Closing remaining comments to submit.
again. Each character incurs overhead from calling ICU, resizing
vectors, intersecting and so on.Steinar H GundersonResizing vectors in this case are the vectors of possible scripts per character? In a separate change, or in this one, can we just keep these vectors pre-allocated or use a fixed maximum size std::array for them?
They are already pre-allocated to the best of our ability; we don't call malloc. But they cannot be fixed-size as long as we want to do stuff like push_front().
Acknowledged
Editor-TipTap [ -5.8%, -5.5%]Does this mostly affect the Latin parts of Editor-TipTap? Or does this optimization hit the CJK part of Editor-TipTap, too? Or is Editor TipTap only the French Guttenberg text and the long programming code by now? I remember it had a CJK text, or did it?
I don't know what goes into Editor-TipTap. On screen, it only shows Latin text, and I can only find that in resources/editors/longtext.html, but I can't guarantee that it doesn't try to shape CJK somewhere off-screen.
I believe the optimization should work well for Chinese (which mainly consists of long strings of Han characters, AFAIK) but less so for Japanese (which frequently mixes kanji and hana), and I don't know enough about hangul in Unicode to say anything about Korean.
Acknowledged
// If we're down to a singleton script, anything that's allowed in thatCan this be moved into a helper function that is labelled as the optimized forward scan?
I'm not sure if that makes sense; it's intimately tied to local variables in the function. What would the signature look like?
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Speed up ScriptRunIterator.
ScriptRunIterator works by keeping a set of potential scripts
for a given run, intersecting it with per-character sets as it goes
until it's down to EOF or zero candidates (at which point it
chooses a single candidate from the previous set, and ends
the run). However, in the vast majority of cases, it will very
quickly end up with a single candidate (e.g. “this text is
definitely in the Latin script”) and just ends up checking
that this script is valid for the next character, over and over
again. Each character incurs overhead from calling ICU, resizing
vectors, intersecting and so on.
Add a fast path so that once we're down at a singleton script,
we consult a bitmap (lazily built, kept per-process) of which
code points are allowed in that script. This allows us to move
through the text in a tight loop (only a single bitmap lookup
per character, plus some bounds checking and loop overhead)
at long as we don't see any non-compatible characters (that would
have us end the run), brackets (which require special handling)
or surrogates.
Speedometer3 (M1 Pinpoint, LTO but no PGO, significant results
at 99% CI only, lower is better except for Score):
TodoMVC-Lit-Complex-DOM [ -0.5%, -0.1%]
Editor-TipTap [ -5.8%, -5.5%]
Score [ +0.2%, +0.5%]
- M third_party/blink/renderer/platform/fonts/script_run_iterator.cc
- M third_party/blink/renderer/platform/fonts/script_run_iterator.h
- M third_party/blink/renderer/platform/fonts/script_run_iterator_test.cc
Code-Review: +1 by Dominik Röttsches
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
luci-bi...@appspot.gserviceaccount.com has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |