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.

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

Simplify processor ordering #2651

Merged
merged 1 commit into from Mar 27, 2024
Merged

Simplify processor ordering #2651

merged 1 commit into from Mar 27, 2024

Conversation

SirYwell
Copy link
Member

Overview

Description

This is a little refactoring for how processors are ordered before being called.

Submitter Checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter Checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
    Options
  2. Ensure that the pull request title represents the desired changelog entry.
    Options
  3. New public fields and methods are annotated with @since TODO.
    Options
  4. I read and followed the contribution guidelines.
    Options

@SirYwell SirYwell requested a review from a team as a code owner March 25, 2024 16:31
@github-actions github-actions bot added the chore label Mar 25, 2024
@@ -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

@dordsor21 dordsor21 requested a review from a team March 27, 2024 09:00
@NotMyFault NotMyFault merged commit 8095111 into main Mar 27, 2024
9 checks passed
@NotMyFault NotMyFault deleted the refactor/processor-list branch March 27, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants