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

Simplify processor ordering #2651

Merged
merged 1 commit into from Mar 27, 2024
Merged

Simplify processor ordering #2651

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 5 additions & 18 deletions ...-core/src/main/java/com/fastasyncworldedit/core/extent/processor/MultiBatchProcessor.java
View file
Open in desktop
Expand Up @@ -19,12 +19,9 @@
import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import java.util.function.Supplier;
Expand Down Expand Up @@ -80,28 +77,18 @@ public void removeBatchProcessor(IBatchProcessor processor) {

@Override
public IChunkSet processSet(IChunk chunk, IChunkGet get, IChunkSet set) {
Map<Integer, Set<IBatchProcessor>> ordered = new HashMap<>();
Map<Integer, List<IBatchProcessor>> ordered = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Could it be worth caching this per processor and marking dirty when changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to profile that code, but I got like 3 traces in that code vs thousands in the processSet calls below. So it's not really even measurable yet noticeable.

The change from HashSet to ArrayList should simplify insertion and iteration logic, but given the measurement difficulty, that's rather theoretical.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm fair enough. Should have been cached in the first place really but I suppose it doesn't have any noticeable performance impact

IChunkSet chunkSet = set;
for (IBatchProcessor processor : processors) {
if (processor.getScope() != ProcessorScope.ADDING_BLOCKS) {
ordered.merge(
processor.getScope().intValue(),
new HashSet<>(Collections.singleton(processor)),
(existing, theNew) -> {
existing.add(processor);
return existing;
}
);
ordered.computeIfAbsent(processor.getScope().intValue(), k -> new ArrayList<>())
.add(processor);
continue;
}
chunkSet = processSet(processor, chunk, get, chunkSet);
}
if (ordered.size() > 0) {
for (int i = 1; i <= 4; i++) {
Set<IBatchProcessor> processors = ordered.get(i);
if (processors == null) {
continue;
}
if (!ordered.isEmpty()) {
for (List<IBatchProcessor> processors : ordered.values()) {
for (IBatchProcessor processor : processors) {
chunkSet = processSet(processor, chunk, get, chunkSet);
if (chunkSet == null) {
Expand Down