lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Edward Ribeiro (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (SOLR-12699) make LTRScoringModel immutable (to allow hashCode caching)
Date Mon, 03 Sep 2018 18:27:00 GMT

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

Edward Ribeiro edited comment on SOLR-12699 at 9/3/18 6:26 PM:
---------------------------------------------------------------

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.

 

Finally, the hashCode can be written as:
{code:java}
result = (prime * result) + Objects.hashCode(features){code}
 

It will automatically return 0 (zero) if features is null or the hash code of the collection
otherwise. 


was (Author: eribeiro):
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


Mime
View raw message