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 <huizhe.wang at oracle.com>
> *Gesendet:* Friday, June 26, 2020 8:29:41 AM
> *An:* Bernd Eckenfels <ecki at zusammenkunft.net>; Core-Libs-Dev 
> <core-libs-dev at openjdk.java.net>
> *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 <core-libs-dev-retn at openjdk.java.net> im Auftrag 
> von Joe Wang <huizhe.wang at oracle.com>
> > Gesendet: Friday, June 26, 2020 1:53:08 AM
> > An: Core-Libs-Dev <core-libs-dev at openjdk.java.net>
> > 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
> >
>