[
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: [email protected]
For additional commands, e-mail: [email protected]
