[jira] [Commented] (SOLR-12699) make LTRScoringModel immutable (to allow hashCode caching)

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (SOLR-12699) make LTRScoringModel immutable (to allow hashCode caching)

JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/SOLR-12699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16646475#comment-16646475 ]

Christine Poerschke commented on SOLR-12699:
--------------------------------------------

Hello [~slivotov] and [~eribeiro] - thank you for progressing this ticket here!

bq. *... if `features` is a large collection this could impact the performance ...*

Good point. The [ManagedModelStore|https://github.com/apache/lucene-solr/blob/releases/lucene-solr/7.5.0/solr/contrib/ltr/src/java/org/apache/solr/ltr/store/rest/ManagedModelStore.java#L236] instantiates the {{LTRScoringModel}} objects and since this would be basically once per the lifetime of the {{SolrCore}} that hopefully would be affordable performance wise. Or looking at it another way, unlike the {{hashCode()}} method calls the object construction does not happen on a per-query basis. Does that kind of make sense?

bq. ... this.params = params != null ? Collections.unmodifiableMap(new HashMap<>(params)) ...

Would suggest to use a {{LinkedHashMap}} so that {{this.params}} and the passed in {{params}} maps have the same ordering. The {{to...Map}} methods in [ManagedModelStore|https://github.com/apache/lucene-solr/blob/releases/lucene-solr/7.5.0/solr/contrib/ltr/src/java/org/apache/solr/ltr/store/rest/ManagedModelStore.java#L272] return a linked hash map and I vaguely recall that there was a reason for that, somehow.

bq. ... Wouldn't be better to make it be `Lists.emptyList()` if `features` is null? Excuse me if I am missing something, but it's usually an anti-pattern to return null, but I am very well aware that the codebases in the wild usually don't follow this advice. ...

Great question, I hadn't really noticed about this before. In practice `features` shouldn't ever actually be null and most (all?) use of `this.features` presumes non-null i.e. would throw a NullPointerException otherwise. In that context {{this.features}} being null if the passed in {{features}} was null is probably clearest i.e. a very apparent NPE pointing towards a code bug of some sort whereas a empty list could be indicative both of a code bug (the 'things' configured by the user were not correctly recognised by the code) or of a misconfiguration issue (the user or tools used to generated the model did not add 'things' as they intended). Likewise for allFeatures, params and norms.

> make LTRScoringModel immutable (to allow hashCode caching)
> ----------------------------------------------------------
>
>                 Key: SOLR-12699
>                 URL: https://issues.apache.org/jira/browse/SOLR-12699
>             Project: Solr
>          Issue Type: Sub-task
>      Security Level: Public(Default Security Level. Issues are Public)
>          Components: contrib - LTR
>            Reporter: Stanislav Livotov
>            Priority: Major
>         Attachments: SOLR-12699.patch, SOLR-12699.patch, SOLR-12699.patch
>
>
> [~slivotov] wrote in SOLR-12688:
> bq. ... LTRScoringModel was a mutable object. It was leading to the calculation of hashcode on each query, which in turn can consume a lot of time ... So I decided to make LTRScoringModel immutable and cache hashCode calculation. ...
> (Please see SOLR-12688 description for overall context and analysis results.)



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]