Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add workaround for KeySpliterator shuffling #198

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add workaround for KeySpliterator shuffling #198

wants to merge 3 commits into from

Conversation

everbrightw
Copy link

@everbrightw everbrightw commented Nov 20, 2024 •
edited
Loading

  • Introduced shuffling in the KeySpliterator of HashMap.

Remaining Work

  • Add shuffling for KeySpliterator
  • Add shuffling for ValueSpliterator
  • Add shuffling for EntrySpliterator

darko-marinov
darko-marinov requested changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@darko-marinov darko-marinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added trivial comments while making a sanity pass.

@kaiyaok2 can you please review this PR in more detail?

nondex-instrumentation/src/main/java/edu/illinois/nondex/instr/HashKeySpliteratorASMDump.java Outdated
return classWriter.toByteArray();
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line at the end?

nondex-core/src/main/java/java/util/KeySpliteratorShuffler.java Outdated
public void forEachRemaining(Consumer<? super K> action) {
iter.forEachRemaining(action);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing end-of-line at the end?

@everbrightw
Copy link
Author

updated for resolving checkstyle

darko-marinov
darko-marinov requested changes Nov 30, 2024
View reviewed changes
Copy link
Contributor

@darko-marinov darko-marinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nondex-instrumentation/src/main/java/edu/illinois/nondex/instr/Instrumenter.java
@@ -72,6 +73,7 @@ public final class Instrumenter {
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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand Spli to full name.

nondex-instrumentation/src/main/java/edu/illinois/nondex/instr/Instrumenter.java
@@ -203,6 +206,13 @@ public byte[] apply() {
}
});
}
// add SpliteratorShuffler.class
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be done only in some cases, like the if-then-else above?

Copy link
Author

@everbrightw everbrightw Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

nondex-core/src/main/java/java/util/HashMap.java
@@ -1559,6 +1567,10 @@ public void forEachRemaining(Consumer<? super K> action) {
}

public boolean tryAdvance(Consumer<? super K> action) {
return shuffler.tryAdvance(action);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good finding that you need to change this method as well.

@everbrightw
Copy link
Author

Added support for instrumenting both tryAdvance and forEachRemaining methods in the Spliterator. Sequential stream uses both tryAdvance and forEachRemaining in different use cases. Previously, instrumenting only forEachRemaining led to undesired behavior, as the original implementation relies on a shared currentIndex to track the pipeline’s progress.

e.g.
forEachRemaining:

keySet.stream().forEach(); // Processes elements in batches

tryAdvance:

keySet.stream().anyMatch(x -> x == 5); // Processes elements one at a time

@everbrightw
Copy link
Author

@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.

@darko-marinov
Copy link
Contributor

Please rerun with the updated version as it'll help (1) automatically remove false alarms due to tryActive and (2) identify which tests are really ID (vs. NOD but just happened to fail by chance). Let me know if you need more VMs to run your experiments.

@everbrightw
Copy link
Author

Access to more VMs would be really helpful! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darko-marinov darko-marinov darko-marinov requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@everbrightw @darko-marinov