apache / apache/lucenenet

Revisit BreakIterator/CharacterIterator for stack operations and reduced allocations

Open
#1,398 0 comments 0 reactions 0 assignees View on GitHub
is:feature pri:low
Dominant language
C#
Stars
2.4k
Forks
658
Avg merge
3d 5h
Merged PRs (30d)
9

Description

> > So, after PR [#1396](https://github.com/apache/lucenenet/pull/1396) that I just opened, that just leaves ICharacterEnumerator/CharacterIterator work. I will analyze that and update here.
>
> @paulirwin - there was an update to the status of this one here: https://github.com/NightOwl888/ICU4N/issues/23#issuecomment-2366826611
>
> The `CharacterIterator` does 3 things for Java users:
>
> 1. Allows existing `BreakIterator` implementations to change their input encoding without any other changes.
> 2. Allows patches for JDK implementations that have broken `CharacterIterator`s.
> 3. Allows Lucene users to extend `CharacterIterator` for other purposes.
>
> Number 2 doesn't seem like a huge concern for .NET. The 3rd is sort of an open question. Given the fact that `CharacterEnumerator` is extended in several places in Lucene, it isn't clear whether users benefit from this extension point or if it only exists to work around JDK bugs.
>
> That said, the fundamental flaw with carrying this `BreakIterator` and `CharacterIterator` design over to .NET is the fact that once you wrap a sequence of characters or bytes with an interface, you lose the ability to perform the operation on the stack. Microsoft ended up just [re-implementing `BreakIterator` segmentation](https://github.com/dotnet/runtime/blob/v10.0.9/src/libraries/System.Private.CoreLib/src/System/Text/Unicode/TextSegmentationUtility.cs) so they could avoid the heap allocation. I haven't fully analyzed that implementation to determine how feasible it would be to make a pluggable `BreakIterator` rules engine fit with that type of design. The real crux of the issue sinks in when you consider how ICU4C supports both UTF-16 and UTF-8 inputs and the fact that Microsoft simply makes these separate inputs into separate static APIs, making it harder to plug a common rules engine into both of them. Unfortunately, there isn't an existing seam in `RuleBasedBreakIterator` that we can take advantage of like was done with `NumberFormatRules` to pull the rules engine into a separate pluggable concept, and it is complicated by the fact that in .NET that `UCultureInfo` contains the data for a given locale rather than being a key to lookup the data as was done in ICU4J.
>
> The current thinking is to continue supporting the `BreakIterator`/`CharacterIterator` in ICU4N while experimenting with the above type of approach to lift segmentation behavior up to the stack. That gives Lucene.NET a window to figure out how to migrate that doesn't necessarily need to be done for the 4.8.0 release. But although I have tried [an experimental `ICharacterEnumerator` implementation](https://github.com/NightOwl888/J2N/blob/v2.1.0/src/J2N/Text/StringCharacterEnumerator.cs) already, it seems like converting it without also changing the design of `BreakIterator` is causing more problems than it solves. The only real "problem" with it is that it exposes the `ICU4N.Support` namespace publicly, but that is simple enough to change for the ICU4N release by moving it into the `ICU4N.Text` namespace and setting it up to be marked as "currently the only way available, but we know this isn't how we want it done" type of API status.
>
> Note that `CharacterIterator` came from the JDK, so it seemed at first like putting it in `J2N` would be the right choice. But given the facts that it works in conjunction with a `BreakIterator` implementation in Lucene (thus requiring a reference to ICU4N anyway) and it actively works against the goal of moving to System.Memory, it seems better to try to phase it out than to support it.
>
> The fact that Lucene.NET extends the `CharacterIterator` interface is just another issue to deal with. Note that for the time being, [ICU4N wraps a `ReadOnlyMemory` instance](https://github.com/NightOwl888/ICU4N/blob/v60.1.0-alpha.440/src/ICU4N/Impl/CSCharacterIterator.cs) for use with `BreakIterator`. Since the only known purpose of these classes in Lucene is to fix JDK bugs that don't exist in .NET, I wonder if it would be best to do away with any public notion of `CharacterIterator` in Lucene.NET and simply expose APIs that accept `string`, `char[]` and `ArraySegment`. We could expose `ReadOnlyMemory` also, but it is a bit more dangerous because it doesn't hold on to a pointer. I haven't yet looked at how feasible that change is. That would potentially prevent Lucene.NET users from attempting to use `CharacterIterator` as a way to extend custom highlighter implementations, giving us the ability to change the design later without breaking public APIs.
>
> It also makes me wonder whether we should marry a stack-based `BreakIterator` (or possibly enumerator) design with stack-based highlighters in the `lucenenet-extensions` repo to experiment with how to redesign the highlighters for use with System.Memory types while still providing the ability to extend them.

_Originally posted by @NightOwl888 in [#279](https://github.com/apache/lucenenet/issues/279#issuecomment-4795170226)_

Contributor guide

Open the contributing guide

Research direction

Start by comparing ICU4N's Impl/CSCharacterIterator.cs with the experimental J2N StringCharacterEnumerator implementation and .NET's TextSegmentationUtility.cs. Trace how Lucene.NET extends CharacterIterator and how BreakIterator and highlighters consume it. Done would require a settled, compatible design for stack-based segmentation and the migration or retention of the public CharacterIterator API.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend-api-design, performance
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.