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

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(...).

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(...).

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.

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

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

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

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)

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?

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)

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.

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.

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

Thanks Adriano!

Thank you for committing the patch

Backport to 2.9 and 3.0.

Chaning title

Patch for 3.0 for reference

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