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
