[Performance] iterate the smaller set during Set.intersect()

[dduan]

For Set's intersect() operation, iterate over the smaller of the two.

This change introduces about 1% overhead in speed for average case a small constant cost, but greatly optimizes worst case.

Benchmark result:

master, -R

small ∩ large * 100 -- 0.268759667873383
large ∩ small * 100 -- 7.67079079151154
small ∩ small * 100 -- 0.279738128185272
large ∩ large * 100 -- 17.1418876051903
total time spent ----- 25.3611761927605

optimized, -R

small ∩ large * 100 -- 0.275687038898468
large ∩ small * 100 -- 0.267830967903137
small ∩ small * 100 -- 0.27216911315918
large ∩ large * 100 -- 17.287498831749
total time spent ----- 18.1031859517097

Note: the benchmark was written for intersectInPlace(), which simply calls intersect(). Tests in *validation-test/stdlib/Set.swift * passes.

[dabrahams]

Do you have any theory about why there's even a 1% slowdown in the average case?

I should note that the implementation before your patch was suboptimal in other ways. It's really a bad idea to convert the other sequence to a Set. We should use underestimateCount() on other instead of building a Set and using count so we are not traversing it twice or creating needless storage. We should (probably) allocate the newSet with an initial capacity equal to the smaller of the two sizes. Care to try that arrangement? I'd be happy to merge this pull request first if you think that's best.

[dduan]

@dabrahams I just ran the benchmark a few more times. It seems that 1% is within normal fluctuation between runs of the same test. Bad reporting here.

[dduan]

@dabrahams My understanding is contains() would consume a singe-pass sequence, therefore we must deal with this somehow. Converting other to Set, therefore, is an OK option. Correct me if I'm wrong.

[dabrahams]

@dduan Regarding the 1%, that's reassuring, and what I would have expected. Regarding converting other to Set, it's "OK" semantically, but it is very wasteful. If other is not a set (e.g. an array) we should treat it as the smaller of the two arguments.

[dduan]

@dabrahams Done!

[florianreinhart]

I think it's better to use min(sequence.underestimateCount(), count) for minimumCapacity. In that way you don't allocate too much memory for the new set, if the sequence is large and the set is small.

[dduan]

Good catch. Fixing now.

[gribozavr]

I'm not sure if requesting capacity is the right thing here. We don't have a guarantee that the result will have that many elements. The result could even be empty.

[florianreinhart]

You are right. I think we should just create an empty set without requesting capacity.

[dduan]

@gribozavr the resulting size could be anywhere between 0 and min(other.count, count).

The closer we allocate towards min(other.count, count), the more we risk wasting space.
The closer we allocate towards 0, the more we risk wasting time resizing the storage.

I don't think there's a "right" way to do this. Before I make the change you suggested, just want to point out this trade off.

[dduan]

@gribozavr @florianreinhart I ran benchmark for both arrangements and here's the result. It only represent the time aspect, but it does confirm my theory above.

newSet = Set<Element>()
small ∩ large * 100 -- 0.313429653644562
large ∩ small * 100 -- 9.32736319303513
small ∩ small * 100 -- 0.30213850736618
large ∩ large * 100 -- 19.0307229161263
total time spent ----- 28.9736542701721

newSet = Set<Element>(minimumCapacity: min(other.count, count))
small ∩ large * 100 -- 0.262056231498718
large ∩ small * 100 -- 9.16748160123825
small ∩ small * 100 -- 0.256339490413666
large ∩ large * 100 -- 15.4998977780342
total time spent ----- 25.1857751011848

[dduan]

@gribozavr If memory is deemed more important than speed here, I'll make the change. Please let me know 😬

[gribozavr]

@dduan Thank you for the benchmarks! Swift is being used on platforms that are memory-constrained, like iOS devices, so yes, my preference is to be conservative and not over-allocate.

For example, we don't reserve space in filter(), even though we have an upper bound on the number of elements. Continuing to do this in intersect() would be consistent with current practices.

[dduan]

@gribozavr Great, I've made the changes. Thank you for clarifying!

[florianreinhart]

@dduan Now you aren't iterating over the smaller collection anymore!? If sequence is a Set you iterate over self. If sequence is not a Set you iterate over sequence.

Instead, I think we should do the following: If sequence is a Set compare the counts and iterate over the smaller Set. And if sequence isn't a Set, just iterate over sequence. Wasn't that your initial idea?

[dduan]

@florianreinhart hmm, restored. Thanks for catching it. I ran the benchmark and tests on Set again just as a sanity check this time.

small ∩ large * 100 -- 0.269078969955444
large ∩ small * 100 -- 0.276514112949371
small ∩ small * 100 -- 0.286798119544983
large ∩ large * 100 -- 17.9460356235504
total time spent ----- 18.7784268260002

@gribozavr

[florianreinhart]

Looks good to me!

[dduan]

@gribozavr any other concerns?

[gribozavr]

@dduan Looks good, thanks!