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

Add `count(where:)` and tests #16099

Merged
merged 5 commits into from Sep 13, 2018

Conversation

Projects
None yet
6 participants
@khanlou
Copy link
Contributor

khanlou commented Apr 23, 2018

This pull request is a reference implementation and tests for the count(where:) function on Sequence. The relevant pitch thread can be found here.

Several notes:

  • There's currently a failing diagnostic test:

    // <rdar://problem/21080030> Bad diagnostic for invalid method call in boolean expression: (_, ExpressibleByIntegerLiteral)' is not convertible to 'ExpressibleByIntegerLiteral
    func rdar21080030() {
      var s = "Hello"
      if s.count() == 0 {} // expected-error{{cannot call value of non-function type 'Int'}}{{13-15=}}
    }
    

    This makes sense, because count is now a property and a function. Not sure what the recommended approach is here: do we delete this test? Do we change the expected error to "wrong number of parameters"?

  • I didn't include count(of:) where Element is Equatable, but I can, depending on what the temperature of the review is. If we do add count(of:), I think we probably also want to add specializations for ranges, sets, etc.

  • count(of:) also might need special consideration for NSCountedSet.

@lattner

This comment has been minimized.

Copy link
Collaborator

lattner commented Aug 15, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Copy link
Collaborator

lattner commented Aug 15, 2018

This proposal has been accepted, please merge it when ready. Thank you for the contribution @khanlou !

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Aug 15, 2018

does it have merge conflicts? (there's no way to for me to see)

@lattner

This comment has been minimized.

Copy link
Collaborator

lattner commented Aug 16, 2018

Seems like it merges fine. @airspeedswift, can you nominate someone to code review and merge this? Thanks.

@airspeedswift airspeedswift self-requested a review Aug 16, 2018

@airspeedswift
Copy link
Member

airspeedswift left a comment

Tests look good, but could you add a benchmark as well? Maybe one on String or Set for some combined coverage.

public func count(
where predicate: (Element) throws -> Bool
) rethrows -> Int {
var count = 0

This comment has been minimized.

@airspeedswift

airspeedswift Aug 16, 2018

Member

Any reason to not use reduce here?

This comment has been minimized.

@khanlou

khanlou Sep 6, 2018

Contributor

I think using a reduce is needlessly complicated both in terms of abstraction and in terms of code readability, which is why I went with the for loop. If you feel strongly, I can switch it to a reduce.

This comment has been minimized.

@airspeedswift

airspeedswift Sep 13, 2018

Member

I disagree. This kind of calculation is exactly what reduce is for. It states declaratively what the code does instead of writing an open-coded for loop, so improves readability. It's also important that the code in the std lib encourage the practice of using map/reduce/filter where appropriate.

(it does bug me you have to write try twice with reduce, tho)

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Aug 16, 2018

re the diagnostic test - you could replace with underestimatedCount. But seems like it isn’t failing any more?

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Aug 16, 2018

@natecook1000 any feedback on the doc comment?

@tkremenek

This comment has been minimized.

Copy link
Member

tkremenek commented Aug 22, 2018

@swift-ci test

@tkremenek

This comment has been minimized.

Copy link
Member

tkremenek commented Aug 22, 2018

Anything left to do here?

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Aug 22, 2018

Build failed
Swift Test OS X Platform
Git Sha - 25b2fd8

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Aug 22, 2018

Build failed
Swift Test Linux Platform
Git Sha - 25b2fd8

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Aug 22, 2018

@tkremenek Ben asked for a performance test, but I don't know how to add that yet, so I've been putting it off.

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Aug 25, 2018

Is there any way for me to see why the build failed? I'm getting a 404.

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Aug 25, 2018

Those aren’t real failures, you get them when you kick off a rerun of the tests sometimes. The tests then ran and passed.

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Sep 6, 2018

I got help from @natecook1000 and am working on the benchmark today and tomorrow. Hopefully will have something soon.

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Sep 7, 2018

Can someone kick off a benchmark run to make sure everything is working right?

@natecook1000

This comment has been minimized.

Copy link
Member

natecook1000 commented Sep 7, 2018

@swift-ci Please smoke benchmark

@natecook1000

This comment has been minimized.

Copy link
Member

natecook1000 commented Sep 7, 2018

@swift-ci Please smoke test

@natecook1000

This comment has been minimized.

Copy link
Member

natecook1000 commented Sep 7, 2018

@swift-ci Please smoke benchmark

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Sep 7, 2018

Build comment file:

Performance: -O

TEST MIN MAX MEAN MAX_RSS
Added
CountAlgoRange 2859 2940 2886
CountAlgoString 3445 3676 3523

Performance: -Osize

TEST MIN MAX MEAN MAX_RSS
Added
CountAlgoRange 5716 5846 5760
CountAlgoString 3437 3569 3482

Performance: -Onone

TEST MIN MAX MEAN MAX_RSS
Added
CountAlgoRange 2564333 2565523 2564903
CountAlgoString 4843 4975 4888
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@natecook1000

This comment has been minimized.

Copy link
Member

natecook1000 commented Sep 7, 2018

Quite a difference between optimized and unoptimized for the range!

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Sep 12, 2018

What should we do about the range benchmark? I'm assuming 40 minutes is too long to include in the suite

@natecook1000

This comment has been minimized.

Copy link
Member

natecook1000 commented Sep 12, 2018

@swift-ci Please smoke benchmark

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Sep 12, 2018

Build comment file:

Performance: -O

TEST MIN MAX MEAN MAX_RSS
Added
CountAlgoArray 575 585 578
CountAlgoString 3429 3612 3490

Performance: -Osize

TEST MIN MAX MEAN MAX_RSS
Added
CountAlgoArray 576 576 576
CountAlgoString 3425 3697 3540

Performance: -Onone

TEST MIN MAX MEAN MAX_RSS
Added
CountAlgoArray 144507 170970 153367
CountAlgoString 4755 4858 4803
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@natecook1000

This comment has been minimized.

Copy link
Member

natecook1000 commented Sep 13, 2018

@swift-ci Please smoke test

@khanlou

This comment has been minimized.

Copy link
Contributor

khanlou commented Sep 13, 2018

Does this PR need anything else?

@natecook1000 natecook1000 merged commit 5736cac into apple:master Sep 13, 2018

3 checks passed

Swift Benchmark on macOS Platform (smoke test) Build finished.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@khanlou khanlou deleted the khanlou:add-count-where branch Sep 13, 2018

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Sep 13, 2018

I raised https://bugs.swift.org/browse/SR-8742 for the weird Range benchmark thing.

dcci added a commit that referenced this pull request Sep 17, 2018

Add `count(where:)` and tests (#16099)
* add count(where:) and tests

* Revise count(where:) documentation

* Remove errant word in abstract

* add a benchmark for ranges and strings with help from @natecook1000

* update benchmark to use Array instead of Range
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment