[OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Phil Race
philip.race at oracle.com
Tue Apr 12 19:59:03 UTC 2016
I am catching up on email here, and "+1" but a couple of comments
- ColorModel.hashCode() doesn't say a lot about how it is calculated
so it seems safe to change it. Sometimes it makes performance sense
to cache the calculated value but in this case there is probably no
big win from doing so since it is not likely a commonly used case.
Also equals looks unavoidably expensive.
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
- As Sergey noted, do file a CCC - this has been general practice when
over-riding equals or hashCode. I am not sure if you will get asked
there to say anything about what is meant by "equals()" or or how
"hashCode()" is calculated. You should get that CCC approved before
committing.
-phil.
On 04/12/2016 12:45 PM, Jim Graham wrote:
> Hi Jay,
>
> Looks great! Good to go...
>
> ...jim
>
> On 4/12/2016 4:36 AM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> I have updated the webrev to include changes to check testBit()
>> instead of using isValid().
>> Please find the updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.03/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Tuesday, April 12, 2016 12:21 AM
>> To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
>> Cc: 2d-dev at openjdk.java.net
>> Subject: Re: Review Request for JDK-7107905: equals() method in
>> IndexColorModel doesnt exist and it relies on ColorModel.equals()
>> which is not strict
>>
>> Hi Jay,
>>
>> There was one thing I pointed out in the first review that got lost
>> in the shuffle. When the validBits are not null you use isValid(i)
>> to test the values, but that method does 3 things:
>>
>> - tests the index for validity, but we already know it is valid
>> - tests validBits for null, but we already know it is not
>> - calls validBits.testBit() - this is the only part we need
>>
>> For performance, I'd switch to just testing for the bits directly, as
>> in:
>>
>> validBits.testBit(i) == cm.validBits.testBit(i)
>>
>> ...jim
>>
>> On 4/11/2016 12:46 AM, Jayathirth D V wrote:
>>> Hi Jim,
>>>
>>> Thanks for the review.
>>> I have made all recommended changes and created new subtask for
>>> JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
>>>
>>> Please review updated webrev:
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Friday, April 08, 2016 12:28 AM
>>> To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
>>> Cc: 2d-dev at openjdk.java.net
>>> Subject: Re: Review Request for JDK-7107905: equals() method in
>>> IndexColorModel doesnt exist and it relies on ColorModel.equals()
>>> which is not strict
>>>
>>> Hi Jayathirth,
>>>
>>> This looks good.
>>>
>>> One thing to note for efficiency, "instanceof" also checks for null.
>>> It only returns true for non-null objects, so you don't need to
>>> explicitly test for null at the top of ICM.equals(). (Though, you
>>> should include a
>>> test(s) in the unit test that verifies that no ICM returns true for
>>> equals(null) to be sure.) You can see that CM.equals() is
>>> implemented this way.
>>>
>>> Also, for performance, ICM should include a quick "if (this == cm)
>>> return true;" check, like CM.equals(). I'd recommend:
>>>
>>> - first instanceof
>>> - then == test
>>> - then super.equals()
>>> - finally, test equality of data fields
>>>
>>> More comments inline:
>>>
>>> On 4/7/16 6:46 AM, Jayathirth D V wrote:
>>>> - Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has
>>>> mentioned the same thing. Can I create a subtask to address
>>>> java.awt.image changes?
>>>
>>> That would be good.
>>>
>>>> For now, it would be good to implement hashCode() on ICM now that
>>>> you are creating an equals() method for it.
>>>>
>>>> - I am not completely sure of what is the optimal way of adding
>>>> hashCode() API so I took help from internet and IDE to make the
>>>> changes. I am including super.hashCode() as it adds uniqueness to ICM.
>>>
>>> You did a great job. To save time in the future, you should all
>>> have copies of the latest version of "Effective Java" by Josh
>>> Bloch. It has a whole chapter on equals/hashCode. It's a very
>>> handy reference for all sorts of good programming practice for the
>>> Java language.
>>>
>>>> Also, ColorModel.hashCode() is a poor implementation. It doesn't
>>>> use the paradigms recommended by Effective Java and looks like it
>>>> produces poor hashes as a result (just in the first two elements of
>>>> the hashCode I see a collision due to poor choice of numbers).
>>>> - I think since we are not using Prime numbers and it might
>>>> result in improper hash code. I have made similar changes to
>>>> hashCode() of ColorModel.
>>>
>>> Looks great.
>>>
>>>> - In the same file itself we are following Java coding
>>>> guidelines of having braces after if like "if () {". I am not
>>>> completely sure of including "{" in new line.
>>>
>>> You are correct, that matching local code is a good consideration
>>> when considering code style. In this case, though, the indentation
>>> on these if statements is an example of what we're trying to avoid
>>> so it would be better to fix these particular statements (don't
>>> bother fixing the other lines in the file at this point, that can
>>> wait until we have to update other parts of the file, but don't
>>> propagate a bad style in new code).
>>> In particular:
>>>
>>> Never do this:
>>>
>>> if (some complex test ||
>>> some additional tests ||
>>> some additional tests ||
>>> some additional tests ||
>>> some continuation of the test) {
>>> the body of the code;
>>> more body of the code;
>>> }
>>> Quick question - where does the body of the if statement start?
>>> Does your eye see it in a fraction of a second or do you have to
>>> search for it?
>>>
>>> That is the worst option for indenting an if statement with
>>> continuation lines. Never do that in new code. Do either of the
>>> following two versions:
>>>
>>> Java Code Style guidelines recommends indenting 8 spaces for
>>> conditional
>>> continuations:
>>> if (some complex test ||
>>> some additional tests ||
>>> some additional tests ||
>>> some additional tests ||
>>> some continuation of the test) {
>>> the body of the code;
>>> more body of the code;
>>> }
>>>
>>> Jim's personal extension to the JCS recommends (and the 2D team
>>> historically tended to agree):
>>> if (some complex test ||
>>> some additional tests ||
>>> some additional tests ||
>>> some additional tests ||
>>> some continuation of the test)
>>> {
>>> the body of the code;
>>> more body of the code;
>>> }
>>>
>>> Both of those immediately draw the eye to the separating point
>>> between the conditional and the body of the code.
>>>
>>>> I'd also add a few test cases that test that 2 separately and
>>>> identically constructed ICM instances are equals() == true, tested
>>>> with one of each of the constructors...
>>>>
>>>> - I have made changes to test case for verifying all
>>>> constructors with same ICM. Also added verification for hashCode
>>>> value.
>>>
>>> Unfortunately, some of your tests for hashCode make an invalid
>>> assumption. It is technically correct for the hash codes of 2
>>> different objects to be the same regardless of whether they are
>>> equals() or not.
>>> In fact, a perfectly valid implementation of hashCode() could return
>>> a constant number and it would be correct from the perspective of
>>> the equals/hashCode contract. (Such code, however, would not be
>>> optimal, but it would be valid/correct-to-spec.) The only condition
>>> that is required is that the hash codes match if the objects are
>>> equals(), but they are allowed to match if the objects are
>>> !equals(). In other words:
>>>
>>> boolean equals1 = (o1.equals(o2));
>>> boolean equals2 = (o2.equals(o1));
>>> boolean equalsH = (o1.hashCode() == o2.hashCode());
>>>
>>> if (equals1 != equals2) that is an error - we should test this if
>>> (equals1 && !equalsH) that is an error - we should test this // No
>>> other conditions are an error, no other testing should be done
>>>
>>> In particular, the cases where you test the hash codes for being the
>>> same on objects that are not intended to be equals() should be deleted.
>>> It would be good to also add tests to make sure that they are
>>> symmetrically equals (or symmetrically not equals) as well (i.e.
>>> o1.equals(o2) matches o2.equals(o1) in all cases whether they match
>>> or not).
>>>
>>> Since it is less than optimal for hash codes to match if the objects
>>> are not equal, it might potentially be something to check on, but it
>>> should not generate a unit test failure and so should not appear in
>>> the unit test for this bug. Such a "code collision test" would be
>>> part of a performance test run periodically for QA, but we have
>>> never bothered with hashCode optimization unless a customer submits
>>> a complaint about the performance of some object in a hash map due
>>> to hash collisions (and I doubt that ColorModel objects are being
>>> used in such a manner), so I wouldn't recommend it here.
>>>
>>> ...jim
>>>
More information about the 2d-dev
mailing list