Committed branch 3.0 revision: 1029022
Committed branch 2.9 revision: 1029027

Patch for 3.0 for reference

Chaning title

Backport to 2.9 and 3.0.

Thank you for committing the patch

Committed trunk revision: 1024408
Committed 3.x revision: 1024409

Thanks Adriano!

There is also no problem with reusing the cloned attribute, as ArrayUtils grows the array correctly also in that case. So you can still apend stuff as usual, with a small perf penalty maybe.

I think I will commit this later this evening to trunk and 3.x. Merging is little harder because of sophisticated backwards layer in 3.x.

Robert is right, in the actual use case, a cloned TermAttribute is never modified, it's at max copied back to the TokenStream's TermAttribute (when using TokenStream.restoreState(State)), so the only TermAttribute instance that needs to grow is the TokenStream's.

The only thing: It still thin it should use an array size calculated by ArrayUtils.oversize() on the clone?

I don't think we should. the clone might never be used again:

So if the clone is never reused, there is no evidence the array will ever grow (most analysis processes, stemming and folding, etc actually shorten the term text)

I have no problem with it.

The only thing: It still thin it should use an array size calculated by ArrayUtils.oversize() on the clone?

Uwe, whats happening with this issue?

Indexing terms are typically small, I think we should commit Adriano's last patch.
I have problems with clone() being slow with -client on my jvm, I think this is a good improvement.

And, can prevent some memory issues (the original intent of the issue it seems)

So, if you guys think my thoughts above are OK, this is a simple patch with code that creates a new internal buffer strictly equals to the term length

I was checking State.clone() method usage and it's just used store the current AttributeSource state for later use, when it's restored by invoking AttributeSource.restoreState(), which only copies the valid chars from the stored state to the current CharTermAttribute object the AttributeSource holds. In the end, I see no reason for stored states (the cloned ones) to hold an internal buffer greater than it needs too, once it will never be actually used/changed by any AttributeSource user.

Please, let me know if I'm missing something.

Thanks,
Adriano Crestani

Hi Uwe,

Thanks for the quick reply, your patch looks good enough for me. I just added that optimization part in case somebody complains about poor performance of arrayCopy for long arrays, but I agree with you, mostly terms are short and wouldn't require such optimization.

+1 to also apply the patch to trunk

This patch optimizes the cloning of the CharTermAttributeImpl internal buffer. It keeps using clone() to clone the internal buffer when CharTermAttribute.length() is at least 150 and at least 75% and of the internal buffer length, otherwise, it uses System.arrayCopy(...) to clone it using CharTermAttribute.length() as the new internal buffer size.
It's performing the optimization, because in some scenarios, like cloning long arrays, clone() is usually faster than System.arrayCopy(...).

Haven't seen your patch yet. I dont know if the two extra calculations rectify the barnching, because terms are mostly short...

If we take your patch, the allocations should in all cases be done with ArrayUtils.oversize() to be consistent with the allocation strategy of the rest of CTA.

Here the patch, I see no problem with applying it to 3.x and trunk.

This patch optimizes the cloning of the CharTermAttributeImpl internal buffer. It keeps using clone() to clone the internal buffer when CharTermAttribute.length() is at least 150 and at least 75% and of the internal buffer length, otherwise, it uses System.arrayCopy(...) to clone it using CharTermAttribute.length() as the new internal buffer size.

It's performing the optimization, because in some scenarios, like cloning long arrays, clone() is usually faster than System.arrayCopy(...).

This java application demonstrates how much memory CharTermAttributeImpl.clone() might consume in some scenarios.