You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by "Christine Poerschke (JIRA)" <ji...@apache.org> on 2018/10/11 14:13:00 UTC
[jira] [Commented] (SOLR-12699) make LTRScoringModel immutable (to
allow hashCode caching)
[ 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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org