Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

On 6/30/2020 7:44 PM, Stuart Marks wrote:


An unconventional patch over an unusual hashcode/equals 
implementation is a bit too controversial. I'd like to propose a new 
patch that focus on the problem at hand, that is re-setting the 
opcode causes the item in HashSet to get lost because of the changed 
hash bucket. This patch therefore simply remove it from the HashSet 
before the change and then add it back as if it's a new item after 
setting the opcode. This patch resolves the issue while leaves BCEL 
6.0's hashCode/equals alone.


Here's the new patch:
http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev02/


Hi Joe,

This makes a good deal more sense than the previous approach! As you 
note this preserves the consistency of equals() and hashCode(), which 
is an important invariant.


It seems a bit roundabout to have to remove something from the 
HashSet, mutate it, and re-add it, but I don't see an alternative. 
Mutating it while in the HashSet is the original cause of problem. 
Trying to contort things so that objects can remain in the HashSet 
while being mutated is difficult in general, and it seems likely to 
lead to additional problems. Given this, removing and re-adding seems 
the most sensible approach.


Indeed, mutating while in the HashSet indicates the code design might 
not have considered it could happen.


It's a bit fragile in that BranchInstruction now has to "know" that 
the InstructionTarget has a HashSet that requires this to be done. 
However, I don't see a way to avoid this without redesigning the 
relationships of the objects in this area -- which seems out of scope 
for this change.


Additional burden for BranchInstruction.  It would have been less 
troublesome if Instructions were not allowed to mutate, that is, 
changing opcode would mean creating new objects. But I think the 
intention is to reduce object creation (that worked for a straight 
forward compilation process).




Just one comment on the test: there are large test data files 
BCELHashCodeTest1.xsl and BCELHashCodeTest2.xsl. What are these? I'm 
not asking for a tutorial, but rather about what function they serve 
relative to this bug. Just a sentence or two about how 
BCELHashCodeTest2.xsl causes (I think) the generated bytecode to 
exceed the 64Kbyte method size limit. Splitting this into multiple 
methods requires mutating some of the instructions, and then hilarity 
ensued. :-)


I'm also not sure that BCELHashCodeTest1.xsl is necessary if it passes 
both with and without the patch. But if you think it's valuable, then 
it's ok for it to stay in.


Will add the comment to the DataProvider and update the webrev.


Aside from this, I think this change is good to go.


Thanks!



(had a nice hike today, and asked the beautiful Lake 22 ;-))


I had to look it up. Lake 22 looks very nice! Please give my regards 
the next time you visit.


It is! The trail itself is nice too with a stream running alongside. 
I'll tell Lake 22 you said hello next time ;-)


-Joe



s'marks

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Stuart Marks]

An unconventional patch over an unusual hashcode/equals implementation is a bit 
too controversial. I'd like to propose a new patch that focus on the problem at 
hand, that is re-setting the opcode causes the item in HashSet to get lost 
because of the changed hash bucket. This patch therefore simply remove it from 
the HashSet before the change and then add it back as if it's a new item after 
setting the opcode. This patch resolves the issue while leaves BCEL 6.0's 
hashCode/equals alone.


Here's the new patch:
http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev02/


Hi Joe,

This makes a good deal more sense than the previous approach! As you note this 
preserves the consistency of equals() and hashCode(), which is an important 
invariant.


It seems a bit roundabout to have to remove something from the HashSet, mutate 
it, and re-add it, but I don't see an alternative. Mutating it while in the 
HashSet is the original cause of problem. Trying to contort things so that 
objects can remain in the HashSet while being mutated is difficult in general, 
and it seems likely to lead to additional problems. Given this, removing and 
re-adding seems the most sensible approach.


It's a bit fragile in that BranchInstruction now has to "know" that the 
InstructionTarget has a HashSet that requires this to be done. However, I don't 
see a way to avoid this without redesigning the relationships of the objects in 
this area -- which seems out of scope for this change.


Just one comment on the test: there are large test data files 
BCELHashCodeTest1.xsl and BCELHashCodeTest2.xsl. What are these? I'm not asking 
for a tutorial, but rather about what function they serve relative to this bug. 
Just a sentence or two about how BCELHashCodeTest2.xsl causes (I think) the 
generated bytecode to exceed the 64Kbyte method size limit. Splitting this into 
multiple methods requires mutating some of the instructions, and then hilarity 
ensued. :-)


I'm also not sure that BCELHashCodeTest1.xsl is necessary if it passes both with 
and without the patch. But if you think it's valuable, then it's ok for it to 
stay in.


Aside from this, I think this change is good to go.


(had a nice hike today, and asked the beautiful Lake 22 ;-))


I had to look it up. Lake 22 looks very nice! Please give my regards the next 
time you visit.


s'marks

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

Hi all,

(had a nice hike today, and asked the beautiful Lake 22 ;-))

An unconventional patch over an unusual hashcode/equals implementation 
is a bit too controversial. I'd like to propose a new patch that focus 
on the problem at hand, that is re-setting the opcode causes the item in 
HashSet to get lost because of the changed hash bucket. This patch 
therefore simply remove it from the HashSet before the change and then 
add it back as if it's a new item after setting the opcode. This patch 
resolves the issue while leaves BCEL 6.0's hashCode/equals alone.


Here's the new patch:
http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev02/

Thanks,
Joe

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

True, It's unusual (refer to some more details in the previous email). 
It's an unusual patch over an unusual design.


IdentifyHashMap might have been a good choice for the mutable types in 
this issue. I'm not sure about other types as the BCEL process does 
involve removing items from the HashSet with a cloned copy. Using 
HashSet and maybe the larger architectural design surrounding it might 
not have been the best. But that's up to the BCEL engineers to decide. 
For the JDK, BCEL has been as stable as it is. I believe only small 
(none-API/archtectural) patches will be considered for the JDK to 
function properly.


Regards,
Joe

On 6/27/2020 4:02 AM, Bernd Eckenfels wrote:
It does not sound aligned if you allow to change the op code which is 
compared in equal() but remember the hashcode.


But I guess it would be best to use a IdendityHashMap if you care for 
the objects. In that case you can implement hashcode uncached and 
aligned with equals. (And not use it)


Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* Joe Wang 
*Gesendet:* Friday, June 26, 2020 8:29:41 AM
*An:* Bernd Eckenfels ; Core-Libs-Dev 

*Betreff:* Re: RFR [16/java.xml] 8248348: Regression caused by the 
update to BCEL 6.0



On 6/25/2020 5:14 PM, Bernd Eckenfels wrote:
> Hello,
>
> What is the advantage of having such a narrow hashcode value space 
compared to the built in idendity hashcode? Would stocking to the 
object idendity not only reduce the footprint, but also make hash 
lookups faster? Without the unclear relationship to the op code?


It's a general contract to override hashCode() when equals() is
overridden, although due to the hacky nature the later was implemented
and also the narrow scope it was applied in the (BCEL) code it didn't
provide more than the Object identity. Still, it's right to keep the
hashCode() method along with equals(), would have allowed all other
subtypes to behave correctly, and beneficial to others who may read the
code (who might wonder why it's missing). It's a bit improvement over no
hashCode().

-Joe
>
> Gruss
> Bernd
>
>
> --
> http://bernd.eckenfels.net
> 
> Von: core-libs-dev  im Auftrag 
von Joe Wang 

> Gesendet: Friday, June 26, 2020 1:53:08 AM
> An: Core-Libs-Dev 
> Betreff: RFR [16/java.xml] 8248348: Regression caused by the update 
to BCEL 6.0

>
> Hi,
>
> Please review a fix to a BCEL regression. At issue was the addition of
> hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
> method was implemented to return the instruction's opcode that
> unfortunately is mutable. The problem hasn't showed up until the code
> path led to calls to remove from a HashSet a target that has been
> changed since it was added to the HashSet. The proposed patch is to make
> the hashCode final/immutable.
>
> This patch implies that a target object is considered the same one even
> if its field values may have been changed. It therefore may not be
> appropriate in other situations (or may cause problems). However, since
> it had always had no hashCode() override before BCEL 6.0, thereby
> relying on Object's identity hash code, its use case has been limited
> and time-tested. It can benefit from this patch in that it provides the
> same function as Object's hash code, and then serves as a reminder (to
> any who might read the code) how it was used (objects considered to be
> the same over the course as far as the hashCode is concerned).
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
> webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/
>
> Thanks,
> Joe
>

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

On 6/26/2020 12:46 PM, Stuart Marks wrote:



On 6/25/20 4:53 PM, Joe Wang wrote:
Please review a fix to a BCEL regression. At issue was the addition 
of hashCode() method to Instruction.java in BCEL 6.0. This hashCode() 
method was implemented to return the instruction's opcode that 
unfortunately is mutable. The problem hasn't showed up until the code 
path led to calls to remove from a HashSet a target that has been 
changed since it was added to the HashSet. The proposed patch is to 
make the hashCode final/immutable.


This patch implies that a target object is considered the same one 
even if its field values may have been changed. It therefore may not 
be appropriate in other situations (or may cause problems). However, 
since it had always had no hashCode() override before BCEL 6.0, 
thereby relying on Object's identity hash code, its use case has been 
limited and time-tested. It can benefit from this patch in that it 
provides the same function as Object's hash code, and then serves as 
a reminder (to any who might read the code) how it was used (objects 
considered to be the same over the course as far as the hashCode is 
concerned).


JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/


Hi Joe,

This fix seems vaguely wrong, but in the context of what I understand 
it's doing, it seems that BCEL or the surrounding JAXP code is even 
more wrong. :-)


The patch is surely unusual, because of this unusual case. As far as 
which one is more wrong, I believe the burden is mostly on BCEL although 
JAXP might share a bit, at the least it exposed the issue.




From what I understand, this bugfix was prompted by a case where an 
Instruction was added to a HashSet. The Instruction was then mutated 
while in the HashSet, and this resulted in it not being removed from 
the HashSet in the future, when it was necessary to do so, thus 
leaving a stale entry. How could this have possibly worked?!?


It works in most majority of the cases, although I think the BCEL 
developers admitted they had limited tests. This is because while 
Instruction allows mutation, all but one type of instructions actually 
do,  and the mutated one don't get removed from the HashSet in a normal 
build-up process. But XML applications can go berserk sometimes, and in 
this case resulted in a method exceeding the max method size. When that 
happens, a rebuild process involved many copies/clones, resetting 
targeters, the later included removing targeters (among them the mutated 
ones) from the HashSet. Given that such a big stylesheet is unusual, it 
makes it an edge case. At least, we haven't received any error reports 
from the 20% who adopted Java 11.




I'm speculating a bit, but I could imagine code like this:

    var inst = new Instruction(ORIG_OPCODE);
    var set = new HashSet();
    set.add(inst);
    ...
    inst.setOpcode(NEW_OPCODE); // (1) mutates inst while in HashSet!
    ...
    set.remove(inst); // (2)

In the current version, the hashCode() is simply the opcode, and 
equals() compares opcodes (and possibly other things). This is correct 
from the standpoint of Instruction itself, as equal items have the 
same hashcode. However, the above code fails because at line (2), the 
remove() method looks in the "wrong" hash bucket and can't find the 
instruction, so it does nothing.


Yes, it will be correct for the majority cases where opcodes don't 
change. Also note that HashSet checks hashcode, and then the key's 
identity before key.equals, which is why this patch works. When the 
build process calls on an instruction to dispose itself, it removes 
itself from the targeter HashSet. But while it is itself, if the 
hashcode has changed, it won't even find itself!  (sort of lost itself) 
(that's too many "itself")




The patch changes the class to compute the hashcode at construction 
time, and makes the hashcode constant, so it "fixes" the above code, 
since the remove() method will look in the same bucket and find the 
desired instance. But now Instruction breaks the hashCode/equals 
contract, since it's possible for two instances to be equal but to 
have different hashcodes. (Consider an Instruction that was created 
with NEW_OPCODE. It can be equal to inst, but its hashcode will differ.)


It would break the hashCode/equals contract if it was implemented 
normally. But this is unusual, the equals implementation was nothing but 
normal: for branch instructions, it always returns false!  Thus, this 
unusual hashCode implementation will not break the contract.




It seems to me that this patch is compensating for broken code 
elsewhere in BCEL or JAXP by adding more brokenness. It could be that 
is ok in this narrow usage context. That is, as far as we know, this 
"shaded" copy of BCEL is used *only* by JAXP and not used in general, 
and it may be that the particular ways that JAXP uses BCEL aren't 
broken (and are indeed fixed) by this change. 

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Bernd Eckenfels]

It does not sound aligned if you allow to change the op code which is compared 
in equal() but remember the hashcode.

But I guess it would be best to use a IdendityHashMap if you care for the 
objects. In that case you can implement hashcode uncached and aligned with 
equals. (And not use it)

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: Joe Wang 
Gesendet: Friday, June 26, 2020 8:29:41 AM
An: Bernd Eckenfels ; Core-Libs-Dev 

Betreff: Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 
6.0



On 6/25/2020 5:14 PM, Bernd Eckenfels wrote:
> Hello,
>
> What is the advantage of having such a narrow hashcode value space compared 
> to the built in idendity hashcode? Would stocking to the object idendity not 
> only reduce the footprint, but also make hash lookups faster? Without the 
> unclear relationship to the op code?

It's a general contract to override hashCode() when equals() is
overridden, although due to the hacky nature the later was implemented
and also the narrow scope it was applied in the (BCEL) code it didn't
provide more than the Object identity. Still, it's right to keep the
hashCode() method along with equals(), would have allowed all other
subtypes to behave correctly, and beneficial to others who may read the
code (who might wonder why it's missing). It's a bit improvement over no
hashCode().

-Joe
>
> Gruss
> Bernd
>
>
> --
> http://bernd.eckenfels.net
> 
> Von: core-libs-dev  im Auftrag von Joe 
> Wang 
> Gesendet: Friday, June 26, 2020 1:53:08 AM
> An: Core-Libs-Dev 
> Betreff: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 
> 6.0
>
> Hi,
>
> Please review a fix to a BCEL regression. At issue was the addition of
> hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
> method was implemented to return the instruction's opcode that
> unfortunately is mutable. The problem hasn't showed up until the code
> path led to calls to remove from a HashSet a target that has been
> changed since it was added to the HashSet. The proposed patch is to make
> the hashCode final/immutable.
>
> This patch implies that a target object is considered the same one even
> if its field values may have been changed. It therefore may not be
> appropriate in other situations (or may cause problems). However, since
> it had always had no hashCode() override before BCEL 6.0, thereby
> relying on Object's identity hash code, its use case has been limited
> and time-tested. It can benefit from this patch in that it provides the
> same function as Object's hash code, and then serves as a reminder (to
> any who might read the code) how it was used (objects considered to be
> the same over the course as far as the hashCode is concerned).
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
> webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/
>
> Thanks,
> Joe
>

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Stuart Marks]

On 6/25/20 4:53 PM, Joe Wang wrote:
Please review a fix to a BCEL regression. At issue was the addition of 
hashCode() method to Instruction.java in BCEL 6.0. This hashCode() method was 
implemented to return the instruction's opcode that unfortunately is mutable. 
The problem hasn't showed up until the code path led to calls to remove from a 
HashSet a target that has been changed since it was added to the HashSet. The 
proposed patch is to make the hashCode final/immutable.


This patch implies that a target object is considered the same one even if its 
field values may have been changed. It therefore may not be appropriate in other 
situations (or may cause problems). However, since it had always had no 
hashCode() override before BCEL 6.0, thereby relying on Object's identity hash 
code, its use case has been limited and time-tested. It can benefit from this 
patch in that it provides the same function as Object's hash code, and then 
serves as a reminder (to any who might read the code) how it was used (objects 
considered to be the same over the course as far as the hashCode is concerned).


JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/


Hi Joe,

This fix seems vaguely wrong, but in the context of what I understand it's 
doing, it seems that BCEL or the surrounding JAXP code is even more wrong. :-)


From what I understand, this bugfix was prompted by a case where an Instruction 
was added to a HashSet. The Instruction was then mutated while in the HashSet, 
and this resulted in it not being removed from the HashSet in the future, when 
it was necessary to do so, thus leaving a stale entry. How could this have 
possibly worked?!?


I'm speculating a bit, but I could imagine code like this:

var inst = new Instruction(ORIG_OPCODE);
var set = new HashSet();
set.add(inst);
...
inst.setOpcode(NEW_OPCODE); // (1) mutates inst while in HashSet!
...
set.remove(inst); // (2)

In the current version, the hashCode() is simply the opcode, and equals() 
compares opcodes (and possibly other things). This is correct from the 
standpoint of Instruction itself, as equal items have the same hashcode. 
However, the above code fails because at line (2), the remove() method looks in 
the "wrong" hash bucket and can't find the instruction, so it does nothing.


The patch changes the class to compute the hashcode at construction time, and 
makes the hashcode constant, so it "fixes" the above code, since the remove() 
method will look in the same bucket and find the desired instance. But now 
Instruction breaks the hashCode/equals contract, since it's possible for two 
instances to be equal but to have different hashcodes. (Consider an Instruction 
that was created with NEW_OPCODE. It can be equal to inst, but its hashcode will 
differ.)


It seems to me that this patch is compensating for broken code elsewhere in BCEL 
or JAXP by adding more brokenness. It could be that is ok in this narrow usage 
context. That is, as far as we know, this "shaded" copy of BCEL is used *only* 
by JAXP and not used in general, and it may be that the particular ways that 
JAXP uses BCEL aren't broken (and are indeed fixed) by this change. But I'd like 
to see some assurance of that. It could be that there are bugs introduced by 
this change that simply haven't been uncovered by the testing we've done yet.


Another approach is to figure out what is mutating the Instruction while it's in 
a HashSet and fix that instead.


Unfortunately, either of these approaches seems to involve an arbitrary amount 
of work. :-(


s'marks

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

Hi Peter,

Yes, they must agree: o1.equals(o2) => o1.hashCode() == o2.hashCode(). I 
think that was the original motivation in BCEL 6.0 to add the hashCode() 
method since before that, the Instruction class overrode equals() but 
not hashCode(), so two Instructions that were equal had different 
hashcode. The InstructionComparator compares opcode in general, and adds 
conditions for certain subtypes, e.g. BranchInstruction (BI) among 
others. What the InstructionComparator does was a complicated matter 
originated from how the whole process was designed. They had given it a 
lot of thoughts, replacing HashSet among other things for example. The 
current form was a compromise reached in BCEL 6.0 after a long 
discussion. One such compromise was that BIs were treated uniquely 
(unequal), allowing targeters to be added to a HashSet as unique items, 
and then be able to be removed by themselves (references). For the later 
to happen, the hashcode must remain the same for the same object.


Regards,
Joe

On 6/25/2020 11:53 PM, Peter Levart wrote:

Hi Joe,

For an object to correctly function as an element of a HashSet, its 
hashCode() and equals() methods must agree: o1.equals(o2) => 
o1.hashCode() == o2.hashCode(). I see equals() method delegates to a 
global InstructionComparator instance which can even change. Suppose 
it does not change. But what does such InstructionComparator compare? 
opcode (among other things)?


Regards, Peter


On Fri, 26 Jun 2020, 01:56 Joe Wang, > wrote:


Hi,

Please review a fix to a BCEL regression. At issue was the
addition of
hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
method was implemented to return the instruction's opcode that
unfortunately is mutable. The problem hasn't showed up until the code
path led to calls to remove from a HashSet a target that has been
changed since it was added to the HashSet. The proposed patch is
to make
the hashCode final/immutable.

This patch implies that a target object is considered the same one
even
if its field values may have been changed. It therefore may not be
appropriate in other situations (or may cause problems). However,
since
it had always had no hashCode() override before BCEL 6.0, thereby
relying on Object's identity hash code, its use case has been limited
and time-tested. It can benefit from this patch in that it
provides the
same function as Object's hash code, and then serves as a reminder
(to
any who might read the code) how it was used (objects considered
to be
the same over the course as far as the hashCode is concerned).

JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/

Thanks,
Joe

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Peter Levart]

Hi Joe,

For an object to correctly function as an element of a HashSet, its
hashCode() and equals() methods must agree: o1.equals(o2) => o1.hashCode()
== o2.hashCode(). I see equals() method delegates to a global
InstructionComparator instance which can even change. Suppose it does not
change. But what does such InstructionComparator compare? opcode (among
other things)?

Regards, Peter


On Fri, 26 Jun 2020, 01:56 Joe Wang,  wrote:

> Hi,
>
> Please review a fix to a BCEL regression. At issue was the addition of
> hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
> method was implemented to return the instruction's opcode that
> unfortunately is mutable. The problem hasn't showed up until the code
> path led to calls to remove from a HashSet a target that has been
> changed since it was added to the HashSet. The proposed patch is to make
> the hashCode final/immutable.
>
> This patch implies that a target object is considered the same one even
> if its field values may have been changed. It therefore may not be
> appropriate in other situations (or may cause problems). However, since
> it had always had no hashCode() override before BCEL 6.0, thereby
> relying on Object's identity hash code, its use case has been limited
> and time-tested. It can benefit from this patch in that it provides the
> same function as Object's hash code, and then serves as a reminder (to
> any who might read the code) how it was used (objects considered to be
> the same over the course as far as the hashCode is concerned).
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
> webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/
>
> Thanks,
> Joe
>
>

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

On 6/25/2020 5:14 PM, Bernd Eckenfels wrote:

Hello,

What is the advantage of having such a narrow hashcode value space compared to 
the built in idendity hashcode? Would stocking to the object idendity not only 
reduce the footprint, but also make hash lookups faster? Without the unclear 
relationship to the op code?


It's a general contract to override hashCode() when equals() is 
overridden, although due to the hacky nature the later was implemented 
and also the narrow scope it was applied in the (BCEL) code it didn't 
provide more than the Object identity. Still, it's right to keep the 
hashCode() method along with equals(), would have allowed all other 
subtypes to behave correctly, and beneficial to others who may read the 
code (who might wonder why it's missing). It's a bit improvement over no 
hashCode().


-Joe


Gruss
Bernd


--
http://bernd.eckenfels.net

Von: core-libs-dev  im Auftrag von Joe Wang 

Gesendet: Friday, June 26, 2020 1:53:08 AM
An: Core-Libs-Dev 
Betreff: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

Hi,

Please review a fix to a BCEL regression. At issue was the addition of
hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
method was implemented to return the instruction's opcode that
unfortunately is mutable. The problem hasn't showed up until the code
path led to calls to remove from a HashSet a target that has been
changed since it was added to the HashSet. The proposed patch is to make
the hashCode final/immutable.

This patch implies that a target object is considered the same one even
if its field values may have been changed. It therefore may not be
appropriate in other situations (or may cause problems). However, since
it had always had no hashCode() override before BCEL 6.0, thereby
relying on Object's identity hash code, its use case has been limited
and time-tested. It can benefit from this patch in that it provides the
same function as Object's hash code, and then serves as a reminder (to
any who might read the code) how it was used (objects considered to be
the same over the course as far as the hashCode is concerned).

JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/

Thanks,
Joe

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Bernd Eckenfels]

Hello,

What is the advantage of having such a narrow hashcode value space compared to 
the built in idendity hashcode? Would stocking to the object idendity not only 
reduce the footprint, but also make hash lookups faster? Without the unclear 
relationship to the op code?

Gruss
Bernd


--
http://bernd.eckenfels.net

Von: core-libs-dev  im Auftrag von Joe 
Wang 
Gesendet: Friday, June 26, 2020 1:53:08 AM
An: Core-Libs-Dev 
Betreff: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

Hi,

Please review a fix to a BCEL regression. At issue was the addition of
hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
method was implemented to return the instruction's opcode that
unfortunately is mutable. The problem hasn't showed up until the code
path led to calls to remove from a HashSet a target that has been
changed since it was added to the HashSet. The proposed patch is to make
the hashCode final/immutable.

This patch implies that a target object is considered the same one even
if its field values may have been changed. It therefore may not be
appropriate in other situations (or may cause problems). However, since
it had always had no hashCode() override before BCEL 6.0, thereby
relying on Object's identity hash code, its use case has been limited
and time-tested. It can benefit from this patch in that it provides the
same function as Object's hash code, and then serves as a reminder (to
any who might read the code) how it was used (objects considered to be
the same over the course as far as the hashCode is concerned).

JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/

Thanks,
Joe

RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

Hi,

Please review a fix to a BCEL regression. At issue was the addition of 
hashCode() method to Instruction.java in BCEL 6.0. This hashCode() 
method was implemented to return the instruction's opcode that 
unfortunately is mutable. The problem hasn't showed up until the code 
path led to calls to remove from a HashSet a target that has been 
changed since it was added to the HashSet. The proposed patch is to make 
the hashCode final/immutable.


This patch implies that a target object is considered the same one even 
if its field values may have been changed. It therefore may not be 
appropriate in other situations (or may cause problems). However, since 
it had always had no hashCode() override before BCEL 6.0, thereby 
relying on Object's identity hash code, its use case has been limited 
and time-tested. It can benefit from this patch in that it provides the 
same function as Object's hash code, and then serves as a reminder (to 
any who might read the code) how it was used (objects considered to be 
the same over the course as far as the hashCode is concerned).


JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/

Thanks,
Joe