Revert "perf: replace ArrayList consumer wheel with LongMap for O(1) keyed removal (#3063)"#3113
Closed
He-Pin wants to merge 1 commit into
Closed
Revert "perf: replace ArrayList consumer wheel with LongMap for O(1) keyed removal (#3063)"#3113He-Pin wants to merge 1 commit into
He-Pin wants to merge 1 commit into
Conversation
…keyed removal (apache#3063)" Motivation: PR apache#3063 was inspired by akkadotnet/akka.net#8264. Although the Pekko implementation was written independently (using scala.collection.mutable .LongMap instead of Dictionary/HashMap), there is concern that the original idea and design framing came from reading the akka.net PR description rather than being derived independently from the Pekko codebase. Until the licensing and provenance question is resolved, revert the change so it can be re-done cleanly from scratch. Modification: Revert commit 28d61c5 ("perf: replace ArrayList consumer wheel with LongMap for O(1) keyed removal (apache#3063)"). The consumer wheel returns from Array[LongMap[Consumer]] back to Array[java.util.ArrayList[Consumer]] with ArrayList.removeIf for keyed lookup. BroadcastHubBenchRunner and the high-consumer HubSpec tests added by that PR are removed; the JMH benchmark is restored to its prior configuration. Result: BroadcastHub behavior returns to its state before apache#3063. No public API or binary compatibility change vs. the current main branch; only the internal LongMap-based wheel introduced by apache#3063 is undone. Tests: - sbt "stream/compile" -> success - sbt "stream-tests/Test/testOnly *HubSpec" -> 48 passed, 0 failed - sbt "stream/mimaReportBinaryIssues" -> no issues - git diff --check origin/main..HEAD -> clean References: Refs apache#3063, Refs akkadotnet/akka.net#8264 - provenance review pending
Member
Author
|
@pjfanning let's merge this to reduce the maintain burden. |
Member
Author
|
He is fine |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
PR #3063 replaced the
BroadcastHubconsumer wheel'sArrayListwithscala.collection.mutable.LongMapfor O(1) keyed removal. The PR description credited akkadotnet/akka.net#8264 as its inspiration.Although the Pekko implementation was written from scratch (using
LongMaprather thanDictionary/HashMap), there is concern that the original idea and design framing — replacing the per-slot consumer collection with a keyed map for O(1) removal — came from reading the akka.net PR description rather than being derived independently from the Pekko codebase. Until the licensing and provenance question is fully resolved, revert the change so it can be re-done cleanly from scratch, with independent motivation and design.Modification
Revert the squash-merged commit
28d61c53684dcd22302560220698d74a7c5da789(PR #3063):BroadcastHubconsumer wheel:Array[LongMap[Consumer]]back toArray[java.util.ArrayList[Consumer]]; keyed lookup usesArrayList.removeIfagain.findAndRemoveConsumer,addConsumer,wakeupIdx,onUpstreamFailure, and theAdvance/NeedWakeupevent handlers are restored to their pre-perf: replace ArrayList consumer wheel with LongMap for O(1) keyed removal #3063 shape.BroadcastHubBenchRunner(standalone benchmark runner added by perf: replace ArrayList consumer wheel with LongMap for O(1) keyed removal #3063) is removed.BroadcastHubBenchmarkis restored to its prior@Paramconfiguration (64,256) and singlebroadcastmethod.HubSpectests added by perf: replace ArrayList consumer wheel with LongMap for O(1) keyed removal #3063 (deliver all elements in order to many consumers,handle many consumers when some cancel mid-stream) are removed.The post-#3072
privatefield style (noprivate[this]) is preserved — only the LongMap data-structure change from #3063 is undone.Result
BroadcastHubbehavior returns to its state before #3063. No public API or binary compatibility change vs. the currentmainbranch — only the internal LongMap-based wheel introduced by #3063 is undone. The optimization can be re-done from scratch in a follow-up PR once the provenance question is settled.Tests
sbt "stream/compile"→ successsbt "stream-tests/Test/testOnly *HubSpec"→ 48 passed, 0 failedsbt "stream/mimaReportBinaryIssues"→ no issuesgit diff --check origin/main..HEAD→ cleanReferences
Refs #3063, Refs akkadotnet/akka.net#8264 — provenance review pending.