[stdlib] Implement SE-0171: Reduce with inout #10976
Merged
Conversation
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
Implement and document `reduce(into:_:)`, with a few notes: - The `initial` parameter was renamed `initialResult` to match the first parameter in `reduce(_:_:)`. - The unnamed `combining` parameter was renamed `updateAccumulatingResult` to try and resemble the naming of the closure parameter in `reduce(_:_:)`. - The closure throws and `reduce(into:_)` re-throws. - This documentation mentions that `reduce(into:_)` is preferred over `reduce(_:_:)` when the result is a copy-on-write type and an example where the result is a dictionary. Add benchmarks for reduce with accumulation into a scalar, an array, and a dictionary. Update expected error message in closures test (since there are now two `reduce` methods, the diagnostic is different).
|
@swift-ci Please test |
|
@swift-ci Please smoke benchmark |
Build comment file:Build failed before running benchmark. |
|
That’s odd. It looks like the benchmarks didn’t find |
|
@d-ronnqvist Can you please add a few unit tests to see if that reproduces the problem (which might be a compiler picking another overload for some reason, and not seeing the new one), otherwise it's the problem with the benchmarking setup. Having unit tests is not a bad idea anyway. Thanks! |
This checks: - The examples from the documentation of `reduce(_:_:)` and `reduce(into:_:)`. - Different levels of inference in the closure arguments being passed. - That the value passed as the `initialResult` remains unmodified.
|
@swift-ci Please clean benchmark |
1 similar comment
|
@swift-ci Please clean benchmark |
|
@swift-ci Please benchmark |
|
@dabrahams @moiseev Benchmarking won't work in this case since it will try to run the benchmark with the pre-merge version of the stdlib, which doesn't have |
|
@swift-ci Please smoke test |
|
@natecook1000 Benchmarking just runs two separate builds: pre- and post- merge. And the new benchmarks are reflected in a special session, since they did not exist-pre-merge, there is nothing to compare to. |
|
The above comment is my understanding of it, anyway. |
|
@swift-ci Please benchmark |
|
@natecook1000 My understanding matches @moiseev's |
|
😬 Yep, I'm totally off-base on the compiler error question! I think it still chokes when the number of benchmarks doesn't match up, but perhaps that got fixed somewhere along the way. |
|
@natecook1000 I've only ever seen it not-choke on those. |
Build comment file:Optimized (O)Regression (5)
Improvement (4)
No Changes (329)
Added (5)
Unoptimized (Onone)Regression (1)
Improvement (4)
No Changes (333)
Added (5)
Hardware Overview |
|
Can we compare the performance of the two forms of reduce, e.g. for accumulating both lightweight ( |
|
@dabrahams That would be great! Is it possible to write a test that does that comparison or did you mean to write benchmarks for each form of reduce and manually compare the numbers? |
|
@d-ronnqvist The former could be done by testing a change that switches tests from one implementation to the other, but I actually think the latter is the better approach. |
The ReduceInto benchmark performs three tasks using both `reduce(_:_)` and `reduce(into:_:)` so that their performance can be compared: 1. Summing an array, reducing to `Int` 2. Filtering an array, reducing to `[Int]` 3. Counting letter frequencies, reducing to `[Character: Int]`
|
I've updated the benchmark to perform three tasks - reducing to I tried to implement the and Both had the same performance so I went with the latter because it uses the same "style" as the dictionary benchmark. |
|
@swift-ci Please smoke benchmark |
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke benchmark |
Build comment file:Build failed before running benchmark. |
|
Same problem as previously: Is there something different in how the benchmark was invoked when it worked compared to the other two times? Is it necessary to |
|
on Tue Jul 18 2017, David Rönnqvist <notifications-AT-i.8713187.xyz> wrote:
Same problem as previously: `error: extraneous argument label 'into:' in call`.
Is there something different in how the benchmark was invoked when it
worked compared to the other two times? Is it necessary to `clean` if
the benchmarks are using new methods? Or does `smoke benchmark` behave
differently than just `benchmark`?
The build system is missing dependencies; if smoke benchmark does an
incremental build it could easily be hitting that problem.
https://bugs.swift.org/browse/SR-4608
…--
-Dave
|
|
If there could be a problem with incremental builds, should we run the benchmarks again on a clean build? (Can anyone give commands to the CI or is it only members who have that permission?) |
|
@moiseev @dabrahams Is there anything more that's needed from this PR or anything more that I can do to help it get merged? Should I squash and/or rebase my changes? As a user I would love to start using this. |
|
@swift-ci Please benchmark |
|
@d-ronnqvist When benchmarks pass, IMO, it's good to go. |
Build comment file:Optimized (O)Regression (4)
Improvement (2)
No Changes (315)
Added (6)
Unoptimized (Onone)Regression (8)
Improvement (5)
No Changes (308)
Added (6)
Hardware Overview |
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.
This PR implements SE-0171: Reduce with inout, which is accepted but not implemented.
Trying to mimic the existing
reduce(_:_:)implementation as closely as possible there are a few differences between this implementation and the proposal that's worth noting:initialparameter was renamedinitialResultto match the first parameter inreduce(_:_:).combiningparameter was renamedupdateAccumulatingResultto try and resemble the naming of the closure parameter inreduce(_:_:).reduce(into:_)re-throws.I've tried to keep the
reduce(into:_:)documentation close to the existingreduce(_:_:)documentation but also mention thatreduce(into:_)is preferred overreduce(_:_:)when the result is a copy-on-write type and include an example where the result is a dictionary.I've added a few benchmarks and updated one expected error message (since there are now two
reducemethods, the diagnostic is different).I didn't know how an implementation like this should be tested and I couldn't find much inspiration from existing reduce testing. (Comments and feedback are appreciated).
Implements SE-0171. I couldn't find a bug number for it in the Swift bug tracker.