[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or
From: Philip Race <philip.race () oracle ! com>
Date: 2017-02-10 5:12:54
Message-ID: 589D4BD6.6040405 () oracle ! com
[Download RAW message or body]
So +1 .. and please update the CCC
-phil.
On 2/9/17, 8:51 PM, Jayathirth D V wrote:
>
> Hi,
>
> Phil & Jim thanks for your review.
>
> I have modified the code to remove the unneeded import of import
> java.util.Objects.
>
> Please find the updated webrev :
>
> http://cr.openjdk.java.net/~jdv/7107905/webrev.19/
> <http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.19/>
>
> Thanks,
>
> Jay
>
> *From:*Phil Race
> *Sent:* Friday, February 10, 2017 3:35 AM
> *To:* Jim Graham; Jayathirth D V; 2d-dev@openjdk.java.net
> *Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both methods
>
> Oh .. my reply was to an off-list email. I did not notice that.
> So I should repeat that here :
>
> On 2/9/17 12:38 PM, Phil Race wrote:
>
> 32 import java.util.Objects;
>
> This is now un-used, isn't it ? Yet all 3 subclasses still have
> this import.
>
> I don't need to "approve" a new webrev containing that but it would
> be good to publish one.
>
> +1
>
>
> -phil.
>
> On 02/09/2017 01:59 PM, Jim Graham wrote:
>
> From my end this looks good. +1 except for 2 outstanding review
> issues:
>
> - Would like to hear back final comments from Joe Darcy on the new
> doc changes/CCC request
> - Phil pointed out that there is an unneeded import in some of the
> files. I agree that we should make a final webrev to delete them,
> but I don't need to approve it if that is the only change...
>
> ...jim
>
> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>
> Hello All,
>
> There was a closed test which was failing because of
> identity-as-equals approach for ColorModel equals() method.
> I have modified it and added in the webrev. Along with this we
> are now using colorspace.hashCode() in hashCode() functions
> instead of Objects.hashCode(this.colorspace). Reverted using
> Arrays.equals() in IndexColorModel equals() method because
> Arrays.copyOf() takes lot of time.
>
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
> <http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.18/>
>
> Ran jtreg test and JCK there are no additional test case
> failures because of the above change. Only 4 JCK tests are
> failing as it was happening previously.
>
> Just copy pasted my observation regarding JCK failures so that
> we can trace it easily:
>
> 1)api/java_awt/Image/ColorModel/index.html#Constructor:
> Failed. test cases: 4; passed: 3; failed: 1; first test case
> failure: ColorModel2001
>
> This test fails because getComponentSize() returned an
> array with length 3 but it expects the length to be 4. In the
> test case they have bits per component array of length 4
> like {8, 8, 8, 8}. But in the test case wherever they are
> passing "has Alpha" as "false" we omit the alpha component
> bit. This is because of tighter check that we have in
> ColorModel class as "nBits = Arrays.copyOf(bits,
> numComponents);" . "numComponents" will be 3 if hasAlpha is
> false.
>
> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed.
> test cases: 3; passed: 2; failed: 1; first test case failure:
> ColorModel0004
>
> Here they check for equality between 2 ColorModel objects
> having same values, but it fails because now we are using
> identity-as-equals check in ColorModel.
>
> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
> test cases: 2; passed: 1; failed: 1; first test case failure:
> ColorModel2006
>
> Here they check for hashCode equality between 2 ColorModel
> objects having same values, but it fails since we don't have
> hashCode check in ColorModel and it will be different
> between 2 Objects.
>
> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1:
> Failed. test cases: 2; passed: 1; failed: 1; first test case
> failure: testCase1
>
> Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This
> is also happening because of same reason as why the first JCK
> test is failing. We omit alpha bit if hasAlpha is false
> but JCK test tries to call getComponentSize() with index 3
> which throws ArrayIndexOutOfBoundsException.
>
> Thanks,
> Jay
> -----Original Message-----
> From: Jayathirth D V
> Sent: Wednesday, February 08, 2017 3:41 PM
> To: Jim Graham; Philip Race; 2d-dev@openjdk.java.net
> <mailto:2d-dev@openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or
> both methods
>
> Hello All,
>
> I have updated the webrev to include the following changes.
>
> 1) Have identity as equals check in equals() method of
> ColorModel but elaborate the specification of equals() and
> hashCode() in ColorModel on what properties to check
> in subclasses of ColorModel.
> 2) Made changes to test case to have single helper method
> wherever we have same equals/hashCode() check.
> 3) Updated IndexColorModel equals() method to use
> Arrays.equals() for rgb[] data.
> 4) Add comment on why we are not using validBits to
> calculate hashCode() in IndexColorModel hashCode() method.
>
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
> <http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.17/>
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, February 02, 2017 2:51 AM
> To: Phil Race; Jayathirth D V; 2d-dev@openjdk.java.net
> <mailto:2d-dev@openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or
> both methods
>
> I think we should move this issue (array size returned from
> getCompSizes) into a separate bug entry and a separate fix.
> I don't think we need to fix the clone() in the constructor
> and the getter just to get hashcode/equals right...
>
> ...jim
>
> On 1/31/17 2:34 PM, Jim Graham wrote:
>
> For an application to run into this same issue they'd have
> to expect
> getCompSizes() to return data for components that don't
> exist. It's
> unlikely they would use that data if they really
> understand the
> objects. While that would be odd, I guess I can see
> someone might be
> constructing all of their CM's from an array of 4 components
> regardless of the number of actual components and we'd be
> happily
> remembering the useless extra components and returning an
> array of 4
> from getCompSizes(). As I said, they shouldn't really be
> reading and
> interpreting those extra components for any image
> processing, but I can imagine that they might do something
> like create a variant CM by calling the CompSizes() and
> copying them into a new array to construct a new CM with
> modifications. They might just robotically always copy 4
> values without really checking how many are valid. That's
> a stretch, and their code is weak. I can conceive of how
> this might happen, but I really have no idea how likely it
> is...
>
> ...jim
>
> On 1/30/17 3:56 PM, Phil Race wrote:
>
> Sounds like we should at least try to get the tests
> updated so they only test what the spec. says.
> Although it does indicate that there is at least a
> chance that
> application code might also fail due to similar
> assumptions.
> Does #1 not fail with the previous iteration of this
> change too ?
>
> -phil.
>
> On 01/30/2017 01:40 PM, Jim Graham wrote:
>
> Hmmm. Sounds like the test cases were written
> based on bugs in the
> implementation. I'm not sure what the best tactic
> is here for the
> short term for getting this in, but many of these
> changes should eventually be considered bugs in
> the tests. Is it acceptable to break API tests
> like this at the last minute even if the tests are
> at fault? Phil?
>
> Notes on specific instances below...
>
> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>
> Hi Phil,
>
>
> 1)api/java_awt/Image/ColorModel/index.html#Constructor:
> Failed.
> test cases: 4; passed: 3; failed: 1; first
> test case failure:
> ColorModel2001
>
> This test fails because getComponentSize()
> returned an array with length 3 but it expects
> the length to be 4. In
> the test case they have bits per component
> array of length 4 like {8, 8, 8, 8}. But
> in the test case wherever
> they are passing "has Alpha" as "false" we
> omit the alpha component bit. This is because
> of tighter check that we
> have in ColorModel class as "nBits =
> Arrays.copyOf(bits,
> numComponents);" . "numComponents" will be 3
> if hasAlpha is false.
>
>
> This is a bug in the test then, especially if the
> size of our array matches the return value of
> getNumComponents.
>
>
>
> 2)api/java_awt/Image/ColorModel/index.html#Equals:
> Failed. test
> cases: 3; passed: 2; failed: 1; first test case
> failure: ColorModel0004
>
> Here they check for equality between 2
> ColorModel objects
> having same values, but it fails because now
> we are using identity-as-equals check in
> ColorModel.
>
>
> How do they accomplish this when the CM class is
> abstract? Do they
> create a relatively empty subclass and instantiate
> that?
>
> The documentation for the equals() method does not
> document the
> conditions under which it returns true, it uses a
> vague concept of "equals this ColorModel". I
> don't see how they could test anything given that
> documentation.
>
>
>
> 3)api/java_awt/Image/ColorModel/index.html#HashCode:
> Failed.
> test cases: 2; passed: 1; failed: 1; first
> test case
> failure: ColorModel2006
>
> Here they check for hashCode equality
> between 2 ColorModel objects having same
> values, but it fails since we
> don't have hashCode check in ColorModel and
> it will be different between 2 Objects.
>
>
> Same as above, there are no promises documented.
>
>
>
> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTest
>
> testCase1: Failed. test cases: 2; passed: 1;
> failed: 1; first test case failure: testCase1
>
> Throws
> "java.lang.ArrayIndexOutOfBoundsException: 3".
> This is also happening because of same reason
> as why the
> first JCK test is failing. We omit alpha bit
> if hasAlpha is false but JCK test tries to
> call getComponentSize()
> with index 3 which throws
> ArrayIndexOutOfBoundsException.
>
>
> Same assessment as #1 above...
>
> Again, while these are my recommendations about
> the correctness of
> these tests, the question remains whether we want
> to introduce a
> change at this point in the release cycle that
> will essentially invalidate a number of tests that
> have been working for several releases already.
> I'll leave that tactic issue to Phil...
>
> ...jim
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
So +1 .. and please update the CCC<br>
<br>
-phil.<br>
<br>
On 2/9/17, 8:51 PM, Jayathirth D V wrote:
<blockquote cite="mid:e1dd27ba-6bc9-49d8-bd78-3cc8111add43@default"
type="cite">
<meta http-equiv="Context-Type" content="text/html;
charset=us-ascii">
<div>
<p><span>Hi, </span></p>
<p><span> </span></p>
<p><span>Phil & Jim thanks for your review. </span></p>
<p><span>I have modified the code to remove the unneeded import
of import java.util.Objects. </span></p>
<p><span> </span></p>
<p><span>Please find the updated webrev : </span></p>
<p><span><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.19/">http://cr.openjdk.java.net/~jdv/7107905/webrev.19/</a>
</span></p>
<p><span> </span></p>
<p><span>Thanks, </span></p>
<p><span>Jay </span></p>
<p><span> </span></p>
<div>
<div>
<p><b><span>From:</span></b><span> Phil Race <br>
<b>Sent:</b> Friday, February 10, 2017 3:35 AM<br>
<b>To:</b> Jim Graham; Jayathirth D V;
<a class="moz-txt-link-abbreviated" \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><br> \
<b>Subject:</b> Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel \
subclasses are missing hashCode() or equals() or both methods </span></p>
</div>
</div>
<p> </p>
<p>Oh .. my reply was to an off-list email. I did not notice
that.<br>
So I should repeat that here :<br>
<br>
On 2/9/17 12:38 PM, Phil Race wrote: <br>
<br>
</p>
<blockquote>
<p>32 import java.util.Objects; <br>
<br>
This is now un-used, isn't it ? Yet all 3 subclasses still
have this import. <br>
<br>
I don't need to "approve" a new webrev containing that but
it would <br>
be good to publish one. <br>
<br>
+1 </p>
</blockquote>
<p><br>
-phil.<br>
<br>
</p>
<div>
<p>On 02/09/2017 01:59 PM, Jim Graham wrote: </p>
</div>
<blockquote>
<p>From my end this looks good. +1 except for 2 outstanding
review issues: <br>
<br>
- Would like to hear back final comments from Joe Darcy on
the new doc changes/CCC request <br>
- Phil pointed out that there is an unneeded import in some
of the files. I agree that we should make a final webrev to
delete them, but I don't need to approve it if that is the
only change... <br>
<br>
...jim \
<br> <br>
On 2/8/17 11:56 PM, Jayathirth D V wrote: <br>
<br>
</p>
<blockquote>
<p>Hello All, <br>
<br>
There was a closed test which was failing because of
identity-as-equals approach for ColorModel equals()
method. <br>
I have modified it and added in the webrev. Along with
this we are now using colorspace.hashCode() in hashCode()
functions instead of Objects.hashCode(this.colorspace).
Reverted using Arrays.equals() in IndexColorModel equals()
method because Arrays.copyOf() takes lot of time. <br>
<br>
Please find updated webrev for review : <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.18/">http://cr.openjdk.java.net/~jdv/7107905/webrev.18/</a>
<br>
<br>
Ran jtreg test and JCK there are no additional test case
failures because of the above change. Only 4 JCK tests are
failing as it was happening previously. <br>
<br>
Just copy pasted my observation regarding JCK failures so
that we can trace it easily: <br>
<br>
1)api/java_awt/Image/ColorModel/index.html#Constructor:
Failed. test cases: 4; passed: 3; failed: 1; first test
case failure: ColorModel2001 <br>
<br>
This test fails because getComponentSize() \
returned an array with length 3 but it expects the length to be 4. In
the test case they have bits per component \
array of length 4 like {8, 8, 8, 8}. But in the test case \
wherever they are passing "has Alpha" as "false" we omit the alpha
component bit. This is because of tighter check \
that we have in ColorModel class as "nBits =
Arrays.copyOf(bits, numComponents);" . "numComponents"
will be 3 if hasAlpha is false. <br>
<br>
2)api/java_awt/Image/ColorModel/index.html#Equals: Failed.
test cases: 3; passed: 2; failed: 1; first test case
failure: ColorModel0004 <br>
<br>
Here they check for equality between 2 \
ColorModel objects having same values, but it fails because now we
are using identity-as-equals check in ColorModel. <br>
<br>
3)api/java_awt/Image/ColorModel/index.html#HashCode:
Failed. test cases: 2; passed: 1; failed: 1; first test
case failure: ColorModel2006 <br>
<br>
Here they check for hashCode equality between 2
ColorModel objects having same values, but it fails since
we don't have hashCode check in ColorModel and \
it will be different between 2 Objects. <br>
<br>
4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1:
Failed. test cases: 2; passed: 1; failed: 1; first test
case failure: testCase1 <br>
<br>
Throws \
"java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of \
same reason as why the
first JCK test is failing. We omit alpha bit if
hasAlpha is false but JCK test tries to call
getComponentSize() with index 3 which throws
ArrayIndexOutOfBoundsException. <br>
<br>
Thanks, <br>
Jay <br>
-----Original Message----- <br>
From: Jayathirth D V <br>
Sent: Wednesday, February 08, 2017 3:41 PM <br>
To: Jim Graham; Philip Race; <a moz-do-not-send="true"
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a>
<br>
Subject: Re: [OpenJDK 2D-Dev] Review Request for
JDK-7107905: ColorModel subclasses are missing hashCode()
or equals() or both methods <br>
<br>
Hello All, <br>
<br>
I have updated the webrev to include the following
changes. <br>
<br>
1) Have identity as equals check in equals() \
method of ColorModel but elaborate the specification of equals() and
hashCode() in ColorModel on what properties \
to check in subclasses of \
ColorModel. <br>
2) Made changes to test case to have single \
helper method wherever we have same equals/hashCode() check. <br>
3) Updated IndexColorModel equals() method to \
use Arrays.equals() for rgb[] data. <br>
4) Add comment on why we are not using \
validBits to calculate hashCode() in IndexColorModel hashCode() method.
<br>
<br>
Please find updated webrev for review : <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.17/">http://cr.openjdk.java.net/~jdv/7107905/webrev.17/</a>
<br>
<br>
Thanks, <br>
Jay <br>
<br>
-----Original Message----- <br>
From: Jim Graham <br>
Sent: Thursday, February 02, 2017 2:51 AM <br>
To: Phil Race; Jayathirth D V; <a moz-do-not-send="true"
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a>
<br>
Subject: Re: [OpenJDK 2D-Dev] Review Request for
JDK-7107905: ColorModel subclasses are missing hashCode()
or equals() or both methods <br>
<br>
I think we should move this issue (array size returned
from getCompSizes) into a separate bug entry and a
separate fix. <br>
I don't think we need to fix the clone() in the
constructor and the getter just to get hashcode/equals
right... <br>
<br>
\
...jim <br> <br>
On 1/31/17 2:34 PM, Jim Graham wrote: <br>
<br>
</p>
<blockquote>
<p>For an application to run into this same issue they'd
have to expect <br>
getCompSizes() to return data for components that don't
exist. It's <br>
unlikely they would use that data if they really
understand the <br>
objects. While that would be odd, I guess I can see
someone might be <br>
constructing all of their CM's from an array of 4
components <br>
regardless of the number of actual components and we'd
be happily <br>
remembering the useless extra components and returning
an array of 4 <br>
from getCompSizes(). As I said, they shouldn't really
be reading and <br>
interpreting those extra components for any image
processing, but I can imagine that they might do
something like create a variant CM by calling the
CompSizes() and copying them into a new array to
construct a new CM with modifications. They might just
robotically always copy 4 values without really checking
how many are valid. That's a stretch, and their code is
weak. I can conceive of how this might happen, but I
really have no idea how likely it is... <br>
<br>
\
...jim <br> <br>
On 1/30/17 3:56 PM, Phil Race wrote: <br>
<br>
</p>
<blockquote>
<p>Sounds like we should at least try to get the tests
updated so they only test what the spec. says. <br>
Although it does indicate that there is at least a
chance that <br>
application code might also fail due to similar
assumptions. <br>
Does #1 not fail with the previous iteration of this
change too ? <br>
<br>
-phil. <br>
<br>
On 01/30/2017 01:40 PM, Jim Graham wrote: <br>
<br>
</p>
<blockquote>
<p>Hmmm. Sounds like the test cases were written
based on bugs in the <br>
implementation. I'm not sure what the best tactic
is here for the <br>
short term for getting this in, but many of these
changes should eventually be considered bugs in the
tests. Is it acceptable to break API tests like
this at the last minute even if the tests are at
fault? Phil? <br>
<br>
Notes on specific instances below... <br>
<br>
On 1/30/17 2:22 AM, Jayathirth D V wrote: <br>
<br>
</p>
<blockquote>
<p>Hi Phil, <br>
<br>
1)api/java_awt/Image/ColorModel/index.html#Constructor:
Failed. <br>
test cases: 4; passed: 3; failed: 1; first test
case failure: <br>
ColorModel2001 <br>
<br>
This test fails because getComponentSize()
returned an array with length 3 but it expects the
length to be 4. In <br>
the test case they have bits per component
array of length 4 like {8, 8, 8, 8}. \
But in the test case wherever <br>
they are passing "has Alpha" as "false" we omit
the alpha component bit. This is because of
tighter check that we <br>
have in ColorModel class as "nBits =
Arrays.copyOf(bits, <br>
numComponents);" . "numComponents" will be 3 if
hasAlpha is false. </p>
</blockquote>
<p><br>
This is a bug in the test then, especially if the
size of our array matches the return value of
getNumComponents. <br>
<br>
<br>
</p>
<blockquote>
<p>
2)api/java_awt/Image/ColorModel/index.html#Equals:
Failed. test <br>
cases: 3; passed: 2; failed: 1; first test case <br>
failure: ColorModel0004 <br>
<br>
Here they check for equality between 2
ColorModel objects <br>
having same values, but it fails because now we
are using identity-as-equals check in ColorModel.
</p>
</blockquote>
<p><br>
How do they accomplish this when the CM class is
abstract? Do they <br>
create a relatively empty subclass and instantiate
that? <br>
<br>
The documentation for the equals() method does not
document the <br>
conditions under which it returns true, it uses a
vague concept of "equals this ColorModel". I don't
see how they could test anything given that
documentation. <br>
<br>
<br>
</p>
<blockquote>
<p>
3)api/java_awt/Image/ColorModel/index.html#HashCode:
Failed. <br>
test cases: 2; passed: 1; failed: 1; first test
case <br>
failure: ColorModel2006 <br>
<br>
Here they check for hashCode equality \
between 2 ColorModel objects having same values, but it
fails since we <br>
don't have hashCode check in ColorModel and \
it will be different between 2 Objects. </p>
</blockquote>
<p><br>
Same as above, there are no promises documented. <br>
<br>
<br>
</p>
<blockquote>
<p><br>
\
4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTest <br>
testCase1: Failed. test cases: 2; passed: 1; <br>
failed: 1; first test case failure: testCase1 <br>
<br>
Throws
"java.lang.ArrayIndexOutOfBoundsException: 3".
This is also happening because of same reason as
why the <br>
first JCK test is failing. We omit alpha bit
if hasAlpha is false but JCK test tries \
to call getComponentSize() <br>
with index 3 which throws
ArrayIndexOutOfBoundsException. </p>
</blockquote>
<p><br>
Same assessment as #1 above... <br>
<br>
Again, while these are my recommendations about the
correctness of <br>
these tests, the question remains whether we want to
introduce a <br>
change at this point in the release cycle that will
essentially invalidate a number of tests that have
been working for several releases already. I'll
leave that tactic issue to Phil... <br>
<br>
\
...jim </p> </blockquote>
<p> </p>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<p> </p>
</div>
</blockquote>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic