Improve the performance of copying CircularBuffer #2059
Merged
Conversation
This file contains 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
Signed-off-by: Si Beaumont <beaumont@apple.com>
|
@swift-nio-bot test perf please |
performance reportbuild id: 108 timestamp: Mon Mar 7 18:13:04 UTC 2022 results
comparison
significant differences found |
|
Nice, that's a solid win of about 20%! |
Lukasa
added
the
patch-version-bump-only
Lukasa
reviewed
Mar 7, 2022
Sources/NIOCore/CircularBuffer.swift
Outdated
| return (self.makeIterator(), buffer.startIndex) | ||
| } | ||
|
|
||
| let indexRanges: [Range<Int>] |
There was a problem hiding this comment.
Hmm, we need to confirm that this doesn't allocate, but I forgot to get you to set limits. One sec.
simonjbeaumont
commented
Mar 7, 2022
There was a problem hiding this comment.
@Lukasa Thanks for the review. Should I run the alloc tests locally or wait for the CI to do it's thang?
|
@simonjbeaumont Looks like Swift was not able to optimise the Array you allocated in the implementation. As we will only ever store one or two ranges, can I propose using some |
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
|
@swift-nio-bot test perf please |
performance reportbuild id: 109 timestamp: Tue Mar 8 14:24:37 UTC 2022 results
comparison
significant differences found |
|
Hm... this seems to have made things worse... I mean, it's still better for this test than |
|
The regressions were there in the other benchmarks. I suspect that many of these benchmarks are not running often enough so they're below the noise floor. |
So where does that leave us. Do we have any confidence that this PR is moving things in the right direction, or do we think it's all noise? |
|
I think there are two options here, but the best one is to probably make a PR that raises the runtime of these various benchmarks to around the hundreds-of-ms runtime. That is a larger runtime that makes it a bit easier for us to observe the performance changes. Is that something you have the bandwidth to do? If not I can try to get to it. |
|
@swift-nio-bot test perf please |
performance reportbuild id: 129 timestamp: Thu Mar 24 15:30:20 UTC 2022 results
comparison
significant differences found |
|
OK, so now we've merged #2063 we have lost the noise which made it look like this PR made things worse. It's mostly stable for things that it shouldn't affect and up to ~10% faster for some benchmarks. The only wrinkle is Looks like for 5.2 there was a single rogue allocation which tripped things: @Lukasa unrelated? |
|
Unrelated I suspect, that number has been a bit aggressively low for a while. Let's just re-run, it should pass. |
|
@swift-nio-bot test this please |
1 similar comment
|
@swift-nio-bot test this please |
Lukasa
approved these changes
Apr 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
#1827 suggests we implement some hooks on Collection and Sequence to get better performance for CircularBuffer to increase performance. #2058 added the benchmarks so we can see the improvement from this change.
Modifications:
Implement
Sequence._copyContentsforCircularBufferin terms of its underlyingContiguousArrayto avoid going through the extra layer of indices.Result:
Copying
CircularBuffershould be faster.