[jira] [Commented] (MATH-786) hashCode in Pair class
[
https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13272223#comment-13272223
]
Gilles commented on MATH-786:
-
{quote}
I'm inclined to agree with James here.
But if it is decided to keep the feature, then the Javadoc must be clarified.
{quote}
# I put on this issue on someone else's behalf.
# Just after I clicked submit issue, I realized the consistency problem: See
my own first comment.[1]
# The discussion went on because _you_ were in favour of caching the hash code,
without allowing the current behaviour (no cache). IMO, not having the flag,
and caching is the most dangerous option.
As for the comments on the clarity of the Javadoc, feel free to make it clearer
for you.
IMHO, the class comment makes it quite obvious (for someone who knows what is
meant by immutable is this context) what the problem could be.
[1] (Initial) Question: Does someone see a way to make Pair truly immutable?
(Expected) Answer: No. It is thus not safe to cache the hash code value.
(Simple) Conclusion: Won't fix.
* I'll remove the cache-related code.
* This issue at least served to point to the deficient behaviour of the class's
hashCode method previous implementation. The current one is better I think.
Please review...
hashCode in Pair class
--
Key: MATH-786
URL: https://issues.apache.org/jira/browse/MATH-786
Project: Commons Math
Issue Type: Improvement
Affects Versions: 3.0
Reporter: Gilles
Assignee: Gilles
Priority: Trivial
Fix For: 3.1
Since Pair is supposed to be an immutable class, couldn't we cache the
hashCode value at construction? That would supposedly make it more
efficient when used in maps.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13271981#comment-13271981 ] James Ring commented on MATH-786: - Feel free to ignore me. I have no interest in this feature and I'd never use it, but: - if you need to cache the hashcode values, just do it in the types that you put in the pair. Some types that know they are immutable already do this (e.g. String) - the API documentation is vague: what do you mean by immutable? Does passing false mean I can mutate the pair itself? I think somebody looking at this API is going to have to dig into the source code to figure out exactly what you mean - it unnecessary pollutes a simple API. Also, why not cache toString()? Why single out hashCode()? This optimization is in the wrong place, it should be left to the participating types - it's a premature optimization: nobody has shown a demonstrable benefit and there is, I believe, a negative effect on the cleanliness of the API (but hey, why start making nice APIs now?) - somebody is going to look at the API and wonder if they should set this. If they choose incorrectly (which is possible because of the poor documentation), they get a surprising bug that they wouldn't otherwise have hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13271992#comment-13271992 ] Sebb commented on MATH-786: --- bq. if you need to cache the hashcode values, just do it in the types that you put in the pair. bq. This optimization is in the wrong place, it should be left to the participating types Very good points. I'm inclined to agree with James here. But if it is decided to keep the feature, then the Javadoc must be clarified. hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13269484#comment-13269484 ] Gilles commented on MATH-786: - bq. Of course the default value of the flag will be true. Probably better to be safe, and thus set the default to false! I also add to the discussion that in most parts of Commons Math, we try to avoid dangerous assumptions (cf. defensive copies). Here we cannot make copies but still can offer both options (assume immutable or not). It is still indeed the users' responsibility to use the object consistently with his stated assumption. And, assuming mutability by default will also preserve compatibility with current behaviour (were the hash code is not cached). hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13269529#comment-13269529 ] Gilles commented on MATH-786: - bq. Do you have a use case at hand that requires this change? It's an optimization suggested by a developer who works with maps. I have no idea how much gain it provides; but it seems that for an application that calls hashCode a lot, that might be useful... I don't understand why you call it premature optimization. If the Javadoc states that the correct behaviour is up to the user, I don't see any real problem; we have other cases where a flag indicates that a reference is stored, and if the user does something he shouldn't, the same problems will be created. What I propose seems the perfect compromise between always assuming immutable (the two Seb's opinion) and never optimize hashCode (your opinion). The default being no optimization so that users are not bitten by surprise. Shall I apply the change? hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13269531#comment-13269531 ] Thomas Neidhart commented on MATH-786: -- See my last sentence, I think your solution is fine. In my comment I wanted to point out that assuming and documenting immutability may still be dangerous, especially when all the millions of Pair implementations out there do it differently. An explicit flag is the way to go then if it is really needed for performance reasons imho. hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13269714#comment-13269714 ] James Ring commented on MATH-786: - -1000 to this proposal, unnecessary complicates the API and there's no demonstrated benefit. Why wouldn't the types in the Pair just cache their own hashcodes if performance is so critical? hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[ https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13269943#comment-13269943 ] Gilles commented on MATH-786: - bq. -1000 to this proposal The proposal is fully backwards-compatible. The API is just extended (one new constructor). What's the problem? hashCode in Pair class -- Key: MATH-786 URL: https://issues.apache.org/jira/browse/MATH-786 Project: Commons Math Issue Type: Improvement Affects Versions: 3.0 Reporter: Gilles Assignee: Gilles Priority: Trivial Fix For: 3.1 Since Pair is supposed to be an immutable class, couldn't we cache the hashCode value at construction? That would supposedly make it more efficient when used in maps. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[
https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13268964#comment-13268964
]
Sébastien Brisard commented on MATH-786:
Is there really a way to make {{Pair}} immutable?
How about we write a big warning in the Javadoc that it is the reponsibility of
the user to make sure that objects passed to {{Pair}} are immutable.
I think we _must_ trust the user on this particular problem. As for internal
uses of {{Pair}} it is _our_ responsibility... I think we can take care of
that!!!
hashCode in Pair class
--
Key: MATH-786
URL: https://issues.apache.org/jira/browse/MATH-786
Project: Commons Math
Issue Type: Improvement
Affects Versions: 3.0
Reporter: Gilles
Assignee: Gilles
Priority: Trivial
Fix For: 3.1
Since Pair is supposed to be an immutable class, couldn't we cache the
hashCode value at construction? That would supposedly make it more
efficient when used in maps.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-786) hashCode in Pair class
[
https://issues.apache.org/jira/browse/MATH-786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13269075#comment-13269075
]
Gilles commented on MATH-786:
-
OK.
Then assuming that it's the user's responsibility to not mutate the passed
references, it seems reasonable to _optionally_ allow the performance gain of
computing the hash code at construction, by having a flag in the constructor's
parameter list:
{noformat}
public Pair(K k, V v, boolean assumeImmutable) {
key = k;
value = v;
immutable = assumeImmutable;
hashCode = computeHashCode();
}
{noformat}
Then, we'd have:
{noformat}
public int hashCode() {
return immutable ? hashCode : computeHashCode();
}
{noformat}
What do you think?
hashCode in Pair class
--
Key: MATH-786
URL: https://issues.apache.org/jira/browse/MATH-786
Project: Commons Math
Issue Type: Improvement
Affects Versions: 3.0
Reporter: Gilles
Assignee: Gilles
Priority: Trivial
Fix For: 3.1
Since Pair is supposed to be an immutable class, couldn't we cache the
hashCode value at construction? That would supposedly make it more
efficient when used in maps.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
