[stdlib] Resolving some FIXME comments on Set type. #20631

LucianoPAlmeida · 2018-11-16T09:40:01Z

This is a very simple PR resolving some FIXME comments on Set.swift as a very first contribution :))

natecook1000 · 2018-11-16T16:47:26Z

Welcome to the Swift open source project, @LucianoPAlmeida! This looks great. Could you change your additions to use 2-space indentation? Once that's done we can kick off a test.

LucianoPAlmeida · 2018-11-17T00:29:34Z

For sure @natecook1000, fixed indentation :))

xwu · 2018-11-17T01:04:52Z

@swift-ci please smoke test

airspeedswift · 2018-11-17T01:06:42Z

@swift-ci please benchmark

swift-ci · 2018-11-17T01:49:49Z

Build comment file:

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeFromNSSetAnyObjectForced	5142	4341	-15.6%	1.18x

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

--------------

LucianoPAlmeida · 2018-12-10T00:18:06Z

Hey guys :))
smoke test pass, but do we need to run validation test here too?

xwu · 2018-12-10T03:59:35Z

stdlib/public/core/Set.swift

@@ -712,7 +712,8 @@ extension Set: SetAlgebra {
  @inlinable
  public func isSubset<S: Sequence>(of possibleSuperset: S) -> Bool
  where S.Element == Element {
-    // FIXME(performance): isEmpty fast path, here and elsewhere.


As noted in the comment, there are places "elsewhere" that could use an isEmpty fast path. Before deleting the fixme, it'd be important to identify where those other places are and either implement the fast path or add the fixme comment there.

For sure @xwu 👍
I've checked the methods on Set and added it in all places that I could find it would be possible :))

I’m not so sure you got everything. Wouldn’t isSuperset benefit from an isEmpty fast path?

I've check this isSuperset, but didn't think it has benefit, is an interesting case because just because we have two cases there

let s: Set<String> = [] print(s.isSuperset(of: [])) //false print(s.isSuperset(of: ["A", "B"])) //true

So to add an isEmpty fast path will require, check also possibleSubset for empty too, but since it is a arbitrary sequence it doens't have an isEmpty and as far as I know we could not do this check on underestimatedCount.

Also looking to the implementation(below) no matter what is the size of possibleSubset if self is empty it will return on the first iteration because contains will return false. So there's no performance problem because it will not iterate unnecessarily. Basically, that is the reason I think is not a benefit :))

public func isSuperset<S: Sequence>(of possibleSubset: __owned S) -> Bool where S.Element == Element { for member in possibleSubset { if !contains(member) { return false } } return true }

Turns out there was more "elsewhere": isStrictSubset and isStrictSuperset with Sequence parameter could probably also benefit form optimizations for isEmpty cases... see #24156 (comment).

@palimondo Sorry I miss those 😅, I'll take a look and I can patch up a PR later today :))

No problem. Please don't go in there now, #21300 is about to drastically change how that's handled and ~~I'm already looking into how to handle isEmpty cases on top of it…~~ it only pays the cheap creation of _Bitset(capacity: self.bucketCount) and will bail out of the Sequence consuming loop ASAP.

I just couldn't resist noting how @xwu's pedantry usually turns out to be right, even though it feels pretty annoying to be on the receiving end of it (speaking from experience 😉).

stdlib/public/core/Set.swift

Co-Authored-By: LucianoPAlmeida <[email protected]>

natecook1000 · 2018-12-11T00:41:07Z

@swift-ci Please smoke test

LucianoPAlmeida · 2018-12-13T01:01:31Z

Only some people can trigger tests, right? Can someone please trigger benchmark here :))

xwu · 2018-12-13T02:10:19Z

@swift-ci Benchmark

xwu · 2018-12-13T02:11:02Z

@swift-ci Please benchmark?

swift-ci · 2018-12-13T03:05:35Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
CountAlgoString	1550	1690	+9.0%	0.92x

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: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 16 GB

--------------

LucianoPAlmeida · 2018-12-14T03:13:31Z

Checking the benchmark/single-source/SetTest.swift found that isDisjoint was not being benchmarked.
Also I've added empty set cases for benchmark too :))

natecook1000 · 2018-12-14T23:21:43Z

@swift-ci Please benchmark

natecook1000 · 2018-12-14T23:21:51Z

@swift-ci Please smoke test

swift-ci · 2018-12-15T00:10:08Z

Build comment file:

Performance: -O

TEST	MIN	MAX	MEAN	MAX_RSS
Added
SetIsDisjointBox0	68002	68602	68333	—
SetIsDisjointBox25	5	5	5	—
SetIsDisjointEmptyBox0	1	1	1	—
SetIsDisjointEmptyInt0	1	1	1	—
SetIsDisjointInt0	29345	29514	29426	—
SetIsDisjointInt100	1	1	1	—
SetIsDisjointInt25	2	2	2	—
SetIsDisjointInt50	2	2	2	—
SetIsSubsetEmptyInt0	220	221	220	—
SetSubtractingEmptyBox0	0	0	0	—
SetSubtractingEmptyInt0	0	0	0	—

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
SetTests.o	64415	74727	+16.0%	0.86x

Performance: -Osize

TEST	MIN	MAX	MEAN	MAX_RSS
Added
SetIsDisjointBox0	84710	85258	84913	—
SetIsDisjointBox25	7	7	7	—
SetIsDisjointEmptyBox0	1	1	1	—
SetIsDisjointEmptyInt0	1	1	1	—
SetIsDisjointInt0	30257	30338	30309	—
SetIsDisjointInt100	2	2	2	—
SetIsDisjointInt25	3	3	3	—
SetIsDisjointInt50	3	3	3	—
SetIsSubsetEmptyInt0	227	233	229	—
SetSubtractingEmptyBox0	0	0	0	—
SetSubtractingEmptyInt0	0	0	0	—

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
SetTests.o	57759	66791	+15.6%	0.86x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
DictionaryKeysContainsCocoa	78	54	-30.8%	1.44x
Added
SetIsDisjointBox0	317297	317781	317579	—
SetIsDisjointBox25	26	26	26	—
SetIsDisjointEmptyBox0	5	5	5	—
SetIsDisjointEmptyInt0	5	5	5	—
SetIsDisjointInt0	135109	135375	135209	—
SetIsDisjointInt100	9	10	9	—
SetIsDisjointInt25	13	13	13	—
SetIsDisjointInt50	13	13	13	—
SetIsSubsetEmptyInt0	499	503	501	—
SetSubtractingEmptyBox0	1	1	1	—
SetSubtractingEmptyInt0	1	1	1	—

✅	Benchmark Check Report
⚠️🔤	`SetIsDisjointEmptyBox0` name is composed of 5 words. _{Split SetIsDisjointEmptyBox0 name into dot-separated groups and variants. See http://bit.ly/BenchmarkNaming}
⛔️⏱	`SetIsDisjointInt0` execution took at least 29103 μs. _{Decrease the workload of SetIsDisjointInt0 by a factor of 32 (100), to be less than 1000 μs.}
⚠️🔤	`SetIsDisjointEmptyInt0` name is composed of 5 words. _{Split SetIsDisjointEmptyInt0 name into dot-separated groups and variants. See http://bit.ly/BenchmarkNaming}
⛔️⏱	`SetIsDisjointBox0` execution took at least 67994 μs. _{Decrease the workload of SetIsDisjointBox0 by a factor of 128 (100), to be less than 1000 μs.}
⚠️🔤	`SetIsSubsetEmptyInt0` name is composed of 5 words. _{Split SetIsSubsetEmptyInt0 name into dot-separated groups and variants. See http://bit.ly/BenchmarkNaming}

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

--------------

lorentey

Nice work; thanks for adding those missing benchmarks!

The benchmark report pointed out two scaling issues (also noted in my comment above); other than those, this looks ready to merge!

lorentey · 2018-12-15T00:22:59Z

benchmark/single-source/SetTests.swift

+    setUpFunction: { blackHole([setAB, setCD]) }),
+  BenchmarkInfo(
+    name: "SetIsDisjointBox0",
+    runFunction: { n in run_SetIsDisjointBox(setOAB, setOCD, true, 5000 * n) },


0-overlap sets are the most expensive for isDisjoint(with:) — these two benchmark cases (SetIsDisjointInt0, SetIsDisjointBox0) should use the same factor of 50 as the others.

Just fixed 👍
Thank's for the review @lorentey :))

natecook1000 · 2018-12-15T01:38:26Z

@swift-ci Please smoke test

palimondo · 2019-01-08T21:03:43Z

For the future reference: new performance tests should land in a separate commit from the actual performance work, so that they can demonstrate the expected improvements. Now we only see the after effect in the Added section of the benchmark results.

I'm working on a fix to the loop multipliers that are too low for most of the newly introduced benchmarks and improvement to the benchmark validation to prevent this kind of mistake in PR #21717.

LucianoPAlmeida added 2 commits November 15, 2018 21:20

Fixing some fixmes on stdlib Set

de2bd7b

Adding @inline attr

96a6b40

Fixing spaces

05bd993

xwu reviewed Dec 10, 2018

View reviewed changes

LucianoPAlmeida added 2 commits December 10, 2018 22:11

Adding isEmpty as fast path in other places where is possible.

15d80de

Quotes on variable name on comment.

dadd251

natecook1000 reviewed Dec 11, 2018

View reviewed changes

stdlib/public/core/Set.swift Outdated Show resolved Hide resolved

Update stdlib/public/core/Set.swift

9cda47f

Co-Authored-By: LucianoPAlmeida <[email protected]>

LucianoPAlmeida added 3 commits December 13, 2018 23:05

Merge remote-tracking branch 'origin/master' into set-fixmes

6618421

Adding benchmark for isDisjoint Set method

52d8370

Adding empty sets to benchmark

478c4d1

lorentey approved these changes Dec 15, 2018

View reviewed changes

Fixing the factor on benchmarks and naming warnings for empty 5 words

7f46d25

natecook1000 merged commit 2bc5623 into swiftlang:master Dec 15, 2018

palimondo mentioned this pull request Jan 8, 2019

[benchmark] BenchmarkDoctor: Lower runtime bound + Set.Empty fixes #21717

Merged

[stdlib] Resolving some FIXME comments on Set type. #20631

[stdlib] Resolving some FIXME comments on Set type. #20631

Uh oh!

Conversation

LucianoPAlmeida commented Nov 16, 2018

Uh oh!

natecook1000 commented Nov 16, 2018

Uh oh!

LucianoPAlmeida commented Nov 17, 2018

Uh oh!

xwu commented Nov 17, 2018

Uh oh!

airspeedswift commented Nov 17, 2018

Uh oh!

swift-ci commented Nov 17, 2018

Build comment file:

Performance: -Onone

Uh oh!

LucianoPAlmeida commented Dec 10, 2018

Uh oh!

xwu Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

xwu Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

palimondo May 7, 2019

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida May 7, 2019

Choose a reason for hiding this comment

Uh oh!

palimondo May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

natecook1000 commented Dec 11, 2018

Uh oh!

LucianoPAlmeida commented Dec 13, 2018

Uh oh!

xwu commented Dec 13, 2018

Uh oh!

xwu commented Dec 13, 2018

Uh oh!

swift-ci commented Dec 13, 2018

Build comment file:

Performance: -O

Uh oh!

LucianoPAlmeida commented Dec 14, 2018

Uh oh!

natecook1000 commented Dec 14, 2018

Uh oh!

natecook1000 commented Dec 14, 2018

Uh oh!

swift-ci commented Dec 15, 2018

Build comment file:

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Uh oh!

lorentey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorentey Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

palimondo May 7, 2019 •

edited

Loading

lorentey left a comment •

edited

Loading

lorentey Dec 15, 2018 •

edited

Loading