Tika Metadata Add performance optimization

[patrickdalla]

Hi @wladimirleite , I'm back to work after my licences/vacation. I opened this discussion based on the performance issue you also noted when implementing #1999, about the bad implementation of metadata addition on multi valued metadata field. In that PR it seems that you found a different approach that do not need multivalue.
But the performance issued still exists, and affects any other parser that needs to add multiple metadata. Have you opened an specific issue to address this problem?

[wladimirleite]

Hi @patrickdalla! Welcome back!
For the particular case I was discussing with @lfcnassif, the conclusion was that there are other bottlenecks and inconveniences besides Tika performance when dealing with multivalued fields with many values.
#2011 was opened to deal with the particular issue and #1999 contains the original discussion, including some test results.

[wladimirleite]

Yes, that discussion was about an specific property.
And yes, my comment about IpedMetadata was just an in-theory observation. I will test it in the particular case I was dealing with.

[patrickdalla]

I guess that as all the objects of the ArrayList are put in an array declared in the scope of IpedMetadata class, that is passed internally to the set method, and As the JVM knows that all of them are to be added in the array, and they won't be modified any more as they are controled by IpedMetadata scope, some JVM behind the scenes optimization can take place adding all of them in a simpler operation.

[patrickdalla]

BTW, after 1h and 3m of execution of the worse implementation, it is informing 4h and 56m of estimated finish time, increasing every second.

[wladimirleite]

I guess that as all the objects of the ArrayList are put in an array declared in the scope of IpedMetadata class, that is passed internally to the set method, and As the JVM knows that all of them are to be added in the array, and they won't be modified any more as they are controled by IpedMetadata scope, some JVM behind the scenes optimization can take place adding all of them in a simpler operation.

I guess that the JVM can't make such kind of optimization.
Looking at Tika code below, when you call (from IpedMetadata) the protected set(String, String[]) it calls add(String, String) many times, which calls a method of the writeFilter.

    public void add(final String name, final String value) {
        writeFilter.add(name, value, metadata);
    }

    protected void set(String name, String[] values) {
        //TODO: optimize this to not copy if all
        //values are to be included "as is"
        if (values != null) {
            metadata.remove(name);
            for (String v : values) {
                add(name, v);
            }
        } else {
            metadata.remove(name);
        }
    }

[wladimirleite]

I ran a test with 120,000 messages, each with ~8,000 members set as Communication:To.
So it had a property with 8K distinct string values, repeated 120K times (and each string value has around 20 characters).

I couldn't reproduce the performance difference you observed.
Using Tika's Metadata, the whole processing took 2543, while using IpedMetadata, it took 2579 seconds.
So basically the same time.

// Using Tika's Metadata, with multiple calls to add(String name, String value)
Metadata meta = new Metadata();
ChatGroup groupChat = (ChatGroup) m.getChat();
for (Long id : groupChat.getMembers()) {
    if (id != m.getFrom().getId())
        meta.add(Message.MESSAGE_TO, e.getContact(id).toString());
}

// Using IpedMetadata, with a single call to set(String name, ArrayList<String> values)
IpedMetadata meta = new IpedMetadata();
ChatGroup groupChat = (ChatGroup) m.getChat();
List<String> values = new ArrayList<String>();
for (Long id : groupChat.getMembers()) {
    if (id != m.getFrom().getId())
        values.add(e.getContact(id).toString());
}
meta.set(Message.MESSAGE_TO, values);

I can try running the exact same test you observed the huge performance difference. Maybe something else is going on.

[patrickdalla]

Right. Thanks for testing. Any way, I think we agree in some point: that original Tika implementation is bad, and that a good solution would be to subclass this metadata to override this. For Sqlite Split PR, at least, this improvement proved mandatory to save many timestamps and locations on same multi value field, be it made in IPED project or in Tika project. So we I will transform this in an issue here. Maybe we can store alls this multiple values in a temp diferent internal field and, when iterating over them to save in lucene index, load them from it. Em qui., 4 de jan. de 2024 23:29, Wladimir Leite ***@***.***> escreveu:

…

I ran a test with 120,000 messages, each with ~8,000 members set as Communication:To. So it would be setting a property with 8K distinct string values, repeated 120K times (each string value has around 20 characters). I couldn't reproduce the performance difference you observed. Using Tika's Metadata the whole processing took *2543*, while using your IpedMetadata, it took *2579* seconds. So basically the same time. // Using Tika's `Metadata` with multiple calls to add(String name, String value)Metadata meta = new Metadata();ChatGroup groupChat = (ChatGroup) m.getChat();for (Long id : groupChat.getMembers()) { if (id != m.getFrom().getId()) meta.add(Message.MESSAGE_TO, e.getContact(id).toString()); } // Using IpedMetadata, with a single call set(String name, ArrayList<String> values)IpedMetadata meta = new IpedMetadata();ChatGroup groupChat = (ChatGroup) m.getChat();List<String> values = new ArrayList<String>();for (Long id : groupChat.getMembers()) { if (id != m.getFrom().getId()) values.add(e.getContact(id).toString()); }meta.set(Message.MESSAGE_TO, values); I can try running the exact same test you observed the huge performance difference. Maybe something else is going on. — Reply to this email directly, view it on GitHub <#2039 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG247S4QRXLLTEILASJNYJTYM5XQPAVCNFSM6AAAAABBJ5EMGWVHI2DSMVQWIX3LMV43SRDJONRXK43TNFXW4Q3PNVWWK3TUHM4DAMJZGI3DM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[wladimirleite]

The problem is that for each added item, it creates a new array. This becomes slow if the number of values reaches a few thousand. (Tika default WriteFilter relevant code below).
Another thing to observe is that later in IPED's processing, a new Metadata object is created (after sorting and removing duplicated values). It will call Metadata.add() again, not IpedMetadata.

private static final MetadataWriteFilter ACCEPT_ALL = new MetadataWriteFilter() {
        public void add(String field, String value, Map<String, String[]> data) {
            String[] values = data.get(field);
            if (values == null) {
                set(field, value, data);
            } else {
                data.put(field, appendValues(values, value));
            }
        }

        private String[] appendValues(String[] values, final String value) {
            if (value == null) {
                return values;
            }
            String[] newValues = new String[values.length + 1];
            System.arraycopy(values, 0, newValues, 0, values.length);
            newValues[newValues.length - 1] = value;
            return newValues;
        }
    };

[lfcnassif]

I can try to implement the optimization directly into Tika project when I return. And also discuss with other Tika committers the possibility to make Metadata class thread safe, since its thread unsafety affected us in the past and I had to create a non ideal SyncMetadata class workaround.

[wladimirleite]

One thing that simplifies a lot is the fact that we do not use filters or Property objects.
So it would be much easier to have our own Metadata subclass than changing it in Tika's project.
But as usual, I think it is better to understand what is going on before taking any action, as what @patrickdalla observed doesn't match what I saw in my tests and looking at Tika's code.

[patrickdalla]

In fact, that was what I was trying to start doing when strangely the bottleneck disappeared in Sqlite Split PR. Em sex., 5 de jan. de 2024 06:15, Patrick Bernardina < ***@***.***> escreveu:

…

Right. Thanks for testing. Any way, I think we agree in some point: that original Tika implementation is bad, and that a good solution would be to subclass this metadata to override this. For Sqlite Split PR, at least, this improvement proved mandatory to save many timestamps and locations on same multi value field, be it made in IPED project or in Tika project. So we I will transform this in an issue here. Maybe we can store alls this multiple values in a temp diferent internal field and, when iterating over them to save in lucene index, load them from it. Em qui., 4 de jan. de 2024 23:29, Wladimir Leite ***@***.***> escreveu: > I ran a test with 120,000 messages, each with ~8,000 members set as > Communication:To. > So it would be setting a property with 8K distinct string values, > repeated 120K times (each string value has around 20 characters). > > I couldn't reproduce the performance difference you observed. > Using Tika's Metadata the whole processing took *2543*, while using your > IpedMetadata, it took *2579* seconds. > So basically the same time. > > // Using Tika's `Metadata` with multiple calls to add(String name, String value)Metadata meta = new Metadata();ChatGroup groupChat = (ChatGroup) m.getChat();for (Long id : groupChat.getMembers()) { > if (id != m.getFrom().getId()) > meta.add(Message.MESSAGE_TO, e.getContact(id).toString()); > } > > // Using IpedMetadata, with a single call set(String name, ArrayList<String> values)IpedMetadata meta = new IpedMetadata();ChatGroup groupChat = (ChatGroup) m.getChat();List<String> values = new ArrayList<String>();for (Long id : groupChat.getMembers()) { > if (id != m.getFrom().getId()) > values.add(e.getContact(id).toString()); > }meta.set(Message.MESSAGE_TO, values); > > I can try running the exact same test you observed the huge performance > difference. Maybe something else is going on. > > — > Reply to this email directly, view it on GitHub > <#2039 (reply in thread)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AG247S4QRXLLTEILASJNYJTYM5XQPAVCNFSM6AAAAABBJ5EMGWVHI2DSMVQWIX3LMV43SRDJONRXK43TNFXW4Q3PNVWWK3TUHM4DAMJZGI3DM> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >