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

[Optimization] avoid copying for intersectInPlace() #419

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@dduan
Collaborator

dduan commented Dec 11, 2015

By droping down to native storage implementation, we can perform in place
intersection without making a copy of the set.

Clears tests in validation-test/stdlib/Set.swift

@dduan dduan changed the title from Set: avoid copy during intersectInPlace to [Optimization] avoid copy during intersectInPlace Dec 11, 2015

@dduan dduan changed the title from [Optimization] avoid copy during intersectInPlace to [Optimization] avoid copy for intersectInPlace() Dec 11, 2015

@nadavrot

This comment has been minimized.

Show comment
Hide comment
@nadavrot

nadavrot Dec 11, 2015

Contributor

@dduan Did you measure the performance of this change? Are you seeing any wins?

Contributor

nadavrot commented Dec 11, 2015

@dduan Did you measure the performance of this change? Are you seeing any wins?

@dduan

This comment has been minimized.

Show comment
Hide comment
@dduan

dduan Dec 11, 2015

Collaborator

@nadavrot to be honest, I only took the hint from the "FIXME" comment and reasoned through the code. I'd love to collect some empirical evidence. Is there any established tools/convention for performance measuring for swift?

Collaborator

dduan commented Dec 11, 2015

@nadavrot to be honest, I only took the hint from the "FIXME" comment and reasoned through the code. I'd love to collect some empirical evidence. Is there any established tools/convention for performance measuring for swift?

+ while bucket < native.capacity {
+ if native.isInitializedEntry(bucket) &&
+ !sequence.contains(native.keyAt(bucket)) {
+ native.destroyEntryAt(bucket)

This comment has been minimized.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

Indentation is 2 space.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

Indentation is 2 space.

This comment has been minimized.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

Please leave a FIXME(performance) comment about the necessity to shrink the storage.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

Please leave a FIXME(performance) comment about the necessity to shrink the storage.

+ var bucket = 0
+ while bucket < native.capacity {
+ if native.isInitializedEntry(bucket) &&
+ !sequence.contains(native.keyAt(bucket)) {

This comment has been minimized.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

SequenceType.contains() consumes the sequence. If our tests didn't catch this, it means this patch needs to add tests that use intersectInPlace() with MinimalSequence as the argument.

To fix this issue, you need to use the _preprocessingPass() method and only query the sequence directly multiple times when it is multi-pass. Otherwise, the sequence needs to be copied to a contiguous array first.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

SequenceType.contains() consumes the sequence. If our tests didn't catch this, it means this patch needs to add tests that use intersectInPlace() with MinimalSequence as the argument.

To fix this issue, you need to use the _preprocessingPass() method and only query the sequence directly multiple times when it is multi-pass. Otherwise, the sequence needs to be copied to a contiguous array first.

+ // to avoid invalidating the index and avoiding a copy.
+ let native = nativeOwner.nativeStorage
+ var bucket = 0
+ while bucket < native.capacity {

This comment has been minimized.

@gribozavr

gribozavr Dec 11, 2015

Collaborator

for bucket in 0..<native.capacity ?

@gribozavr

gribozavr Dec 11, 2015

Collaborator

for bucket in 0..<native.capacity ?

@gribozavr

This comment has been minimized.

Show comment
Hide comment
@gribozavr

gribozavr Dec 11, 2015

Collaborator

@dduan Thank you for the patch. I support this change, but we need some changes to make sure we keep the code testable and tested.

Please take a look at this comment in the file:

   //
   // APIs below this comment should be implemented strictly in terms of
   // *public* APIs above.  `_variantStorage` should not be accessed directly.
   //
   // This separates concerns for testing.  Tests for the following APIs need
   // not to concern themselves with testing correctness of behavior of
   // underlying storage (and different variants of it), only correctness of the
   // API itself.
   //

Your change needs to move the method to a different category, and expand tests for this method to be more extensive (that is, to test with both native and bridged sets).

Collaborator

gribozavr commented Dec 11, 2015

@dduan Thank you for the patch. I support this change, but we need some changes to make sure we keep the code testable and tested.

Please take a look at this comment in the file:

   //
   // APIs below this comment should be implemented strictly in terms of
   // *public* APIs above.  `_variantStorage` should not be accessed directly.
   //
   // This separates concerns for testing.  Tests for the following APIs need
   // not to concern themselves with testing correctness of behavior of
   // underlying storage (and different variants of it), only correctness of the
   // API itself.
   //

Your change needs to move the method to a different category, and expand tests for this method to be more extensive (that is, to test with both native and bridged sets).

@dduan

This comment has been minimized.

Show comment
Hide comment
@dduan

dduan Dec 11, 2015

Collaborator

@gribozavr thanks for the comments. I'll make the changes when I get the time.

Collaborator

dduan commented Dec 11, 2015

@gribozavr thanks for the comments. I'll make the changes when I get the time.

@nadavrot

This comment has been minimized.

Show comment
Hide comment
@nadavrot

nadavrot Dec 11, 2015

Contributor

@dduan At the moment there is no good way to test the performance of your patch. Can you write a small benchmark program and test the performance before/after? Adding code to the standard library has a cost (in terms of readability and compile time) and we need to have at least on example that shows a performance win.

@gribozavr

Contributor

nadavrot commented Dec 11, 2015

@dduan At the moment there is no good way to test the performance of your patch. Can you write a small benchmark program and test the performance before/after? Adding code to the standard library has a cost (in terms of readability and compile time) and we need to have at least on example that shows a performance win.

@gribozavr

@dduan

This comment has been minimized.

Show comment
Hide comment
@dduan

dduan Dec 11, 2015

Collaborator

@nadavrot will do!

Collaborator

dduan commented Dec 11, 2015

@nadavrot will do!

@dduan

This comment has been minimized.

Show comment
Hide comment
@dduan

dduan Dec 12, 2015

Collaborator

@nadavrot I ran this program and the speed improvement is around 42% percent (no joke!). Here's the output from my late 2013 MBP with i7:

unoptimized, —debug-swift:
small ∩ large * 100 -- 0.291758000850677
large ∩ small * 100 -- 7.83746302127838
small ∩ small * 100 -- 0.297053933143616
large ∩ large * 100 -- 18.2310382127762
total time spent ----- 26.6573131680489

optimized, —debug-swift
small ∩ large * 100 -- 0.0793769359588623
large ∩ small * 100 -- 7.71100395917892
small ∩ small * 100 -- 0.0776850581169128
large ∩ large * 100 -- 7.35030424594879
total time spent ----- 15.2183701992035
Collaborator

dduan commented Dec 12, 2015

@nadavrot I ran this program and the speed improvement is around 42% percent (no joke!). Here's the output from my late 2013 MBP with i7:

unoptimized, —debug-swift:
small ∩ large * 100 -- 0.291758000850677
large ∩ small * 100 -- 7.83746302127838
small ∩ small * 100 -- 0.297053933143616
large ∩ large * 100 -- 18.2310382127762
total time spent ----- 26.6573131680489

optimized, —debug-swift
small ∩ large * 100 -- 0.0793769359588623
large ∩ small * 100 -- 7.71100395917892
small ∩ small * 100 -- 0.0776850581169128
large ∩ large * 100 -- 7.35030424594879
total time spent ----- 15.2183701992035
@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Dec 12, 2015

Collaborator

Whoa!

Collaborator

lattner commented Dec 12, 2015

Whoa!

Set: avoid copy during intersectInPlace
By droping down to native storage implementation, we can perform in place
intersection without making a copy of the set.

@dduan dduan changed the title from [Optimization] avoid copy for intersectInPlace() to [Optimization] avoid copying for intersectInPlace() Dec 12, 2015

@dduan

This comment has been minimized.

Show comment
Hide comment
@dduan

dduan Dec 12, 2015

Collaborator

@gribozavr I think I've addressed most of your comments. Care to take another look?

@lattner right back at you!

Collaborator

dduan commented Dec 12, 2015

@gribozavr I think I've addressed most of your comments. Care to take another look?

@lattner right back at you!

@nadavrot

This comment has been minimized.

Show comment
Hide comment
@nadavrot

nadavrot Dec 12, 2015

Contributor

@dduan Thank you. The numbers look excellent!

Contributor

nadavrot commented Dec 12, 2015

@dduan Thank you. The numbers look excellent!

@dduan

This comment has been minimized.

Show comment
Hide comment
@dduan

dduan Dec 16, 2015

Collaborator

Closing this one for now. If done right, optimization here would also speed up removeAll(). See discussion on another PR for detail.

Thanks @nadavrot @gribozavr

Collaborator

dduan commented Dec 16, 2015

Closing this one for now. If done right, optimization here would also speed up removeAll(). See discussion on another PR for detail.

Thanks @nadavrot @gribozavr

@dduan dduan closed this Dec 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment