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

["Edward Ribeiro (JIRA)" <ji...@apache.org>]

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

Edward Ribeiro commented on SOLR-12699:
---------------------------------------

Hi [~cpoerschke] and [~slivotov],

Just a nitpick: wrapping a collection in a Collections.unmodifiable* doesn't make the collection strictly immutable. See, we are receiving `features` as a parameter. Therefore, some code that still has a reference to that object can change it underneath. Example:
{code:java}
List<String> mutableList = new ArrayList<>();
mutableList.add("Hello");
mutableList.add("World");

List<String> immutableList = Collections.unmodifiableList(mutableList);

System.out.println("immutableList = " + immutableList);

mutableList.add("!");

System.out.println("list2 = " + immutableList);{code}
 

As collections is a wrapper I could change it by modifying the `mutableList`. AFAIK, to make it a real immutable collection it would be required to create a defensive copy of the collection like below:
{code:java}
List<String> immutableList = Collections.unmodifiableList(new ArrayList<>(mutableList));{code}
 

Now the elements of mutableList are copied to a new ArrayList that we don't have reference. Therefore we cannot change its elements even tough as you pointed out, we could change fields of the objects in the collection. *The problem with this approach is that if `features` is a large collection this could impact the performance, so be cautions when using it.*

Finally, this line
{code:java}
this.features = features != null ? Collections.unmodifiableList(features) : features;{code}
 

`this.features` receives `null` if `features` is `null`. Is that the intended behaviour? 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.

 

> 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