ICU-21388 Use a read-only alias string in ucol_openElements and ucol_setText #1471

jefgen · 2020-11-18T06:25:37Z

The ICU APIs ucol_openElements and ucol_setText both are documented as requiring the caller to not modify or change the input string while the respective object is being used to iterate over the text.
(The API docs say "The caller must not modify or delete the text..." while they are being used. See https://github.com/unicode-org/icu/blob/master/icu4c/source/i18n/unicode/ucoleitr.h#L106 for example.)

In other words, they are documented as using a read-only alias of the input string from the caller.

Both functions already use the UnicodeString's read-only aliasing constructor for the input string arguments. However, some of the implementation methods that they call actually end up unfortunately making full copies of the input string, which hurts performance.

This change adds internal read-only alias methods to the CollationElementIterator and RuleBasedCollator classes, so that we can avoid making copies of the input string for the ucol_openElements and ucol_setText APIs.

This helps to improve the performance of not just the ucol_openElements and ucol_setText APIs, but also the usearch_* API, as it relies on the two previous APIs.

Note: This change is ported from some performance changes that I made a few months ago in the branch here.
This PR is for this commit: microsoft/icu@e6301ff

I thought that splitting up the commits into separate PRs would be easier to review the changes.
Link to other PRs with perf changes: #1472 #1473

I made these changes while investigating some performance issues for .NET Core. The JIRA ticket has some more info on the observed performance improvements seen when running some of the .NET Core benchmarks (with all of the changes).
[cc: @tarekgh as FYI.]

Checklist

Issue filed: https://unicode-org.atlassian.net/browse/ICU-21388
Updated PR title and link in previous line to include Issue number
Issue accepted
Tests included
Documentation is changed or added

…setText The ICU APIs ucol_openElements and ucol_setText both are documented as using a read-only alias of the input string from the caller. (The API docs say that "The caller must not modify or delete the text..." while they are being used.) Both functions already use the UnicodeString's read-only aliasing constructor for the input string arguments. However, the implementation methods that they call actually end up making full copies of the input string, which hurts performance. This change adds internal read-only alias methods to the CollationElementIterator and RuleBasedCollator classes, so that we can avoid making copies of the input string for the ucol_openElements and ucol_setText APIs.

tarekgh · 2020-11-18T17:40:18Z

@jefgen does this optimization can affect the scenario when using the Collator in multiple threads in same time?

In reply to: 729840245

jefgen · 2020-11-18T23:07:52Z

does this optimization can affect the scenario when using the Collator in multiple threads in same time?

The CollationElementIterator internally uses a const Collator:
https://github.com/unicode-org/icu/blob/master/icu4c/source/i18n/unicode/coleitr.h#L366

And the usearch_openFromCollator API also uses a const Collator as well:
https://github.com/unicode-org/icu/blob/master/icu4c/source/i18n/unicode/usearch.h

IIUC, I believe that const Collators can indeed be shared/used across multiple threads -- so long as only const methods are used on the Collators. i.e.: You can't share a Collator with other threads and and also mutate it at the same time.
(Based on reading the User Guide here: https://unicode-org.github.io/icu/userguide/design.html#thread-safe-const-apis).

So I think this optimization would apply regardless of if the Collator is used by itself, or with multiple threads.
(@tarekgh: I think that hopefully answers your question (?), but please let me know if I might have missed it).

In reply to: 730013353

markusicu · 2020-11-23T23:24:51Z

icu4c/source/i18n/coleitr.cpp

+        // Note: This class predates the change in ICU 2.4, which made the UnicodeString's 
+        // assignment operator and copy constructor always allocate and copy even for
+        // readonly aliases.
+        // We could possibly *always* use a readonly alias here with fastCopyFrom, but the


Hm. I am sure that the code was intended to take a string by reference/alias, but yes, this is what the code has been doing for a long time.

Could you please submit a ticket for new public API that only keeps a reference? We could call it setTextReference(). We should also propose a new RBC factory method that creates an iterator with not text at all.

markusicu · 2020-11-23T23:28:06Z

icu4c/source/i18n/coleitr.cpp

@@ -272,12 +272,38 @@ void CollationElementIterator::setOffset(int32_t newOffset,
 */
 void CollationElementIterator::setText(const UnicodeString& source,
                                       UErrorCode& status)
+{
+    setText(source, false, status);


I think we should document on setText(string) that it makes a copy of the string. Assuming we can't safely change the behavior, you could do so in this PR.

Alternatively, you could submit a ticket for that and send an API proposal.

(I think it's safer to just document how it's been for a long time.)

markusicu · 2020-11-23T23:30:57Z

icu4c/source/i18n/rulebasedcollator.cpp

@@ -1631,6 +1631,19 @@ RuleBasedCollator::createCollationElementIterator(const UnicodeString& source) c
    return cei;
 }

+// Same as the above method, but uses a real-only alias of the input source string.


Would you mind checking the inside-ICU call sites of the existing function? I suspect that they should all call this new one.

markusicu · 2020-11-24T00:04:41Z

icu4c/source/i18n/unicode/coleitr.h

+    * @param status the error code status.
+    * @internal
+    */
+    void setText(const UnicodeString &str, UBool readOnly, UErrorCode &status);


Please rename this function to internalSetText(...).

Actually, I think it would be nicer to have separate functions, setText() which continues to make a copy, and internalSetTextReference(string, status) which does the fastCopyFrom() keeping the alias. More readable than a boolean parameter.

markusicu · 2020-11-24T00:07:08Z

icu4c/source/i18n/unicode/coleitr.h

+    * @param order         the collation object.
+    * @param status        the error code status.
+    */
+    CollationElementIterator(const UnicodeString& sourceText, UBool readOnly,


Why not just add the readOnly parameter to the existing constructor? Too many call sites to modify? Might be a good opportunity to review them to see which needs what...

Another possibility is to not change the constructor nor add a new one at all. Instead, call sites could construct with an empty string and then call the new setter.

markusicu · 2020-11-24T00:08:10Z

icu4c/source/i18n/unicode/tblcoll.h

+     *         the based Collator.
+     * @internal
+     */
+    virtual CollationElementIterator* createCollationElementIteratorReadOnlyAlias(


Please prefix the function name with "internal".

markusicu · 2020-11-24T00:08:47Z

icu4c/source/i18n/unicode/tblcoll.h

+     *         the based Collator.
+     * @internal
+     */
+    virtual CollationElementIterator* createCollationElementIteratorReadOnlyAlias(


Does it have to be virtual? If it does, you can't HIDE it because you would create an incompatible vtable.

I think a good alternative is to not add anything here. Change call sites to pass in an empty string and then call the new text setter. We can look at optimizing a CEI with no string / an empty string.

markusicu · 2020-11-24T00:30:45Z

FYI good catch for finding the unintended copy (even if we have to live with it in existing C++ API) -- thanks!

markusicu · 2021-03-23T19:31:04Z

Please modify this PR to use https://unicode-org.atlassian.net/browse/ICU-21533
Getting this into ICU 70 would be lovely. Maybe even early?

markusicu · 2021-09-17T21:33:29Z

@jefgen ping :-)

Please modify this PR to use https://unicode-org.atlassian.net/browse/ICU-21533
Getting this into ICU 70 would be lovely. Maybe even early?

Note that you have another PR for ICU-21533. If that PR gets into ICU 70 but this PR slips past it, then we need to split the ticket again.

jefgen · 2021-09-22T18:50:00Z

Thank you for all the feedback on this PR!

Note that you have another PR for ICU-21533. If that PR gets into ICU 70 but this PR slips past it, then we need to split the ticket again.

Thanks Markus. Unfortunately haven't had time to work on this... I'll need to split the ticket and create a new one for ICU 71.

EDIT: Created a new ticket here: https://unicode-org.atlassian.net/browse/ICU-21760

markusicu · 2022-01-27T00:18:09Z

Hi @jefgen

Thank you for all the feedback on this PR!

Note that you have another PR for ICU-21533. If that PR gets into ICU 70 but this PR slips past it, then we need to split the ticket again.

Thanks Markus. Unfortunately haven't had time to work on this... I'll need to split the ticket and create a new one for ICU 71.

EDIT: Created a new ticket here: https://unicode-org.atlassian.net/browse/ICU-21760

Could you please retarget this PR for ICU-21760 and finish it for 71?

jefgen · 2022-02-14T18:35:21Z

Could you please retarget this PR for ICU-21760 and finish it for 71?

Unfortunately, I don't think I'll have time to get this in for ICU 71.

Instead of keeping this PR open and punting it from release to release, I'm going to close this PR for now. There are no other commits on the ticket ICU-21760, so I'll move it "future", until there is more time to work on this.

jefgen requested review from markusicu, axelandrejs and daniel-ju November 18, 2020 06:25

jefgen assigned markusicu Nov 18, 2020

jefgen mentioned this pull request Nov 18, 2020

fastMode for search and set text #1446

Closed

5 tasks

tarekgh mentioned this pull request Nov 19, 2020

List of performance regressions caused by switching to ICU dotnet/runtime#40942

Open

This was referenced Nov 20, 2020

ICU-21388 Remove test case that does nothing so RuleBasedCollator can be marked as final. #1472

Merged

ICU-21533 Lazily create the internal break iterator used in StringSearch & improve error handling #1473

Merged

markusicu reviewed Nov 24, 2020

View reviewed changes

markusicu added the waiting-on-author label Dec 4, 2020

Base automatically changed from master to main March 24, 2021 17:56

jefgen closed this Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-21388 Use a read-only alias string in ucol_openElements and ucol_setText #1471

ICU-21388 Use a read-only alias string in ucol_openElements and ucol_setText #1471

jefgen commented Nov 18, 2020 •

edited

Loading

tarekgh commented Nov 18, 2020 •

edited by jefgen

Loading

jefgen commented Nov 18, 2020 •

edited

Loading

markusicu Nov 23, 2020

markusicu Nov 23, 2020

markusicu Nov 23, 2020

markusicu Nov 24, 2020

markusicu Nov 24, 2020

markusicu Nov 24, 2020

markusicu Nov 24, 2020

markusicu commented Nov 24, 2020

markusicu commented Mar 23, 2021

markusicu commented Sep 17, 2021

jefgen commented Sep 22, 2021 •

edited

Loading

markusicu commented Jan 27, 2022

jefgen commented Feb 14, 2022

ICU-21388 Use a read-only alias string in ucol_openElements and ucol_setText #1471

ICU-21388 Use a read-only alias string in ucol_openElements and ucol_setText #1471

Conversation

jefgen commented Nov 18, 2020 • edited Loading

Checklist

tarekgh commented Nov 18, 2020 • edited by jefgen Loading

jefgen commented Nov 18, 2020 • edited Loading

markusicu Nov 23, 2020

Choose a reason for hiding this comment

markusicu Nov 23, 2020

Choose a reason for hiding this comment

markusicu Nov 23, 2020

Choose a reason for hiding this comment

markusicu Nov 24, 2020

Choose a reason for hiding this comment

markusicu Nov 24, 2020

Choose a reason for hiding this comment

markusicu Nov 24, 2020

Choose a reason for hiding this comment

markusicu Nov 24, 2020

Choose a reason for hiding this comment

markusicu commented Nov 24, 2020

markusicu commented Mar 23, 2021

markusicu commented Sep 17, 2021

jefgen commented Sep 22, 2021 • edited Loading

markusicu commented Jan 27, 2022

jefgen commented Feb 14, 2022

jefgen commented Nov 18, 2020 •

edited

Loading

tarekgh commented Nov 18, 2020 •

edited by jefgen

Loading

jefgen commented Nov 18, 2020 •

edited

Loading

jefgen commented Sep 22, 2021 •

edited

Loading