Add workaround for KeySpliterator shuffling#198
Add workaround for KeySpliterator shuffling#198everbrightw wants to merge 3 commits intoTestingResearchIllinois:masterfrom
Conversation
darko-marinov
left a comment
darko-marinov
left a comment
There was a problem hiding this comment.
I've added trivial comments while making a sanity pass.
@kaiyaok2 can you please review this PR in more detail?
| return classWriter.toByteArray(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Extra empty line at the end?
| public void forEachRemaining(Consumer<? super K> action) { | ||
| iter.forEachRemaining(action); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing end-of-line at the end?
|
updated for resolving |
darko-marinov
left a comment
darko-marinov
left a comment
There was a problem hiding this comment.
Thanks for adding tryAdvance. I've added some minor comments. I hope Kaiyao can review more, but you should also collect some experimental results while we review.
| public static final String hashMapNodeName = "java/util/HashMap$Node.class"; | ||
| public static final String hashMapEntryName = "java/util/HashMap$Entry.class"; | ||
| public static final String hashMapHashIteratorShufflerName = "java/util/HashMap$HashIterator$HashIteratorShuffler.class"; | ||
| public static final String hashMapKeySpliShufflerName = "java/util/HashMap$KeySpliterator$KeySpliteratorShuffler.class"; |
There was a problem hiding this comment.
Expand Spli to full name.
| } | ||
| }); | ||
| } | ||
| // add SpliteratorShuffler.class |
There was a problem hiding this comment.
Should this be done only in some cases, like the if-then-else above?
There was a problem hiding this comment.
I can confirm for this key case, we do not have to do this.
We probably will need this for the EntrySpliterator, but I can look into this to confirm further (for EntrySpliterator).
| } | ||
|
|
||
| public boolean tryAdvance(Consumer<? super K> action) { | ||
| return shuffler.tryAdvance(action); |
There was a problem hiding this comment.
Good finding that you need to change this method as well.
|
Added support for instrumenting both e.g. tryAdvance: |
|
@darko-marinov Thanks for the review. I’ve finished running real projects at scale from Kaiyao’s repo and manually reviewed some of the newly identified tests, and I discovered this issue. I may need to rerun the experiments with this updated version. |
|
Please rerun with the updated version as it'll help (1) automatically remove false alarms due to |
|
Access to more VMs would be really helpful! Thanks! |
2 participants
KeySpliteratorofHashMap.Remaining Work
KeySpliteratorValueSpliteratorEntrySpliterator