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