Optimize SEL search efficiency #1055

zhongwuzw · 2018-01-20T16:43:49Z

Changes in this pull request

Optimize SEL search efficiency, reduced the time complexity from O (n) to O (1).

Not need tests.

Checklist

All tests pass. Demo project builds and runs.
I added tests, an experiment, or detailed why my change isn't tested.
I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
I have reviewed the contributing guide

facebook-github-bot · 2018-01-20T16:44:18Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2018-01-20T16:58:51Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

jessesquires

😎 Seems reasonable to me! Thanks @zhongwuzw !

cc @rnystrom - any reason you can think of not to do this?

jessesquires · 2018-01-20T18:14:05Z

Source/Internal/IGListAdapterProxy.m

@@ -12,30 +12,34 @@
 #import <IGListKit/IGListAssert.h>

 /**
- Define messages that you want the IGListAdapter object to intercept. Pattern copied from
- https://github.com/facebook/AsyncDisplayKit/blob/7b112a2dcd0391ddf3671f9dcb63521f554b78bd/AsyncDisplayKit/ASCollectionView.mm#L34-L53
+ Define messages that you want the IGListAdapter object to intercept.


let's keep the original comment.

jessesquires · 2018-01-20T18:14:17Z

Source/Internal/IGListAdapterProxy.m

-            sel == @selector(scrollViewDidEndDragging:willDecelerate:) ||
-            sel == @selector(scrollViewDidEndDecelerating:)
-            );
+    static NSSet<NSString *> *sels;


let's spell-out selectors

facebook-github-bot · 2018-01-21T01:31:47Z

@zhongwuzw has updated the pull request. View: changes

iglistkit-bot · 2018-01-21T11:01:43Z

	1 Warning
⚠️	All pull requests should have a milestone attached, unless marked #trivial.

Generated by 🚫 Danger

rnystrom · 2018-01-22T19:40:04Z

This makes sense to me, though I'm curious @zhongwuzw did you do any instrumentation on the efficiency of this? Just curious what the cost of the linear pointer equality checks vs acquiring a lock + lookup is against each other?

I ask b/c even though this is straightforward, I'm skeptical about making changes w/out evidence that it's actually better.

zhongwuzw · 2018-01-23T04:36:24Z

@rnystrom , 👍 , honestly, I did not test it before, just based on my theoretical knowledge. After your comment, I tested like below,

The output:

Compare using pointer check:
0.210490
0.226435
0.232305
0.278115
Compare using NSSet:
1.165643
0.947413
1.003125
0.999605

Conclusion:
NSSet seems not good vs pointer compare. So maybe we should still use pointer to check. But I want to update the order of these three protocol, put UIScrollViewDelegate methods to front, because the times of call are more than others. 😄

static BOOL isInterceptedSelector(SEL sel) {
    static NSSet<NSString *> *selectors;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        selectors = [NSSet setWithObjects:
                     // UICollectionViewDelegate
                     NSStringFromSelector(@selector(collectionView:didSelectItemAtIndexPath:)),
                     NSStringFromSelector(@selector(collectionView:willDisplayCell:forItemAtIndexPath:)),
                     NSStringFromSelector(@selector(collectionView:didEndDisplayingCell:forItemAtIndexPath:)),
                     NSStringFromSelector(@selector(collectionView:didHighlightItemAtIndexPath:)),
                     NSStringFromSelector(@selector(collectionView:didUnhighlightItemAtIndexPath:)),
                     // UICollectionViewDelegateFlowLayout
                     NSStringFromSelector(@selector(collectionView:layout:sizeForItemAtIndexPath:)),
                     NSStringFromSelector(@selector(collectionView:layout:insetForSectionAtIndex:)),
                     NSStringFromSelector(@selector(collectionView:layout:minimumInteritemSpacingForSectionAtIndex:)),
                     NSStringFromSelector(@selector(collectionView:layout:minimumLineSpacingForSectionAtIndex:)),
                     NSStringFromSelector(@selector(collectionView:layout:referenceSizeForFooterInSection:)),
                     NSStringFromSelector(@selector(collectionView:layout:referenceSizeForHeaderInSection:)),
                     // UIScrollViewDelegate
                     NSStringFromSelector(@selector(scrollViewDidScroll:)),
                     NSStringFromSelector(@selector(scrollViewWillBeginDragging:)),
                     NSStringFromSelector(@selector(scrollViewDidEndDragging:willDecelerate:)),
                     NSStringFromSelector(@selector(scrollViewDidEndDecelerating:)),
                     nil];
    });
    return [selectors containsObject:NSStringFromSelector(sel)];
}

static BOOL isInterceptedSelector1(SEL sel) {
    return (
            // UICollectionViewDelegate
            sel == @selector(collectionView:didSelectItemAtIndexPath:) ||
            sel == @selector(collectionView:willDisplayCell:forItemAtIndexPath:) ||
            sel == @selector(collectionView:didEndDisplayingCell:forItemAtIndexPath:) ||
            sel == @selector(collectionView:didHighlightItemAtIndexPath:) ||
            sel == @selector(collectionView:didUnhighlightItemAtIndexPath:) ||
            // UICollectionViewDelegateFlowLayout
            sel == @selector(collectionView:layout:sizeForItemAtIndexPath:) ||
            sel == @selector(collectionView:layout:insetForSectionAtIndex:) ||
            sel == @selector(collectionView:layout:minimumInteritemSpacingForSectionAtIndex:) ||
            sel == @selector(collectionView:layout:minimumLineSpacingForSectionAtIndex:) ||
            sel == @selector(collectionView:layout:referenceSizeForFooterInSection:) ||
            sel == @selector(collectionView:layout:referenceSizeForHeaderInSection:) ||
            // UIScrollViewDelegate
            sel == @selector(scrollViewDidScroll:) ||
            sel == @selector(scrollViewWillBeginDragging:) ||
            sel == @selector(scrollViewDidEndDragging:willDecelerate:) ||
            sel == @selector(scrollViewDidEndDecelerating:)
            );
}

- (void)benchmark{
    NSSet *selectors;
    selectors = [NSSet setWithObjects:
                 // UICollectionViewDelegate
                 NSStringFromSelector(@selector(collectionView:didSelectItemAtIndexPath:)),
                 NSStringFromSelector(@selector(collectionView:willDisplayCell:forItemAtIndexPath:)),
                 NSStringFromSelector(@selector(collectionView:didEndDisplayingCell:forItemAtIndexPath:)),
                 NSStringFromSelector(@selector(collectionView:didHighlightItemAtIndexPath:)),
                 NSStringFromSelector(@selector(collectionView:didUnhighlightItemAtIndexPath:)),
                 // UICollectionViewDelegateFlowLayout
                 NSStringFromSelector(@selector(collectionView:layout:sizeForItemAtIndexPath:)),
                 NSStringFromSelector(@selector(collectionView:layout:insetForSectionAtIndex:)),
                 NSStringFromSelector(@selector(collectionView:layout:minimumInteritemSpacingForSectionAtIndex:)),
                 NSStringFromSelector(@selector(collectionView:layout:minimumLineSpacingForSectionAtIndex:)),
                 NSStringFromSelector(@selector(collectionView:layout:referenceSizeForFooterInSection:)),
                 NSStringFromSelector(@selector(collectionView:layout:referenceSizeForHeaderInSection:)),
                 // UIScrollViewDelegate
                 NSStringFromSelector(@selector(scrollViewDidScroll:)),
                 NSStringFromSelector(@selector(scrollViewWillBeginDragging:)),
                 NSStringFromSelector(@selector(scrollViewDidEndDragging:willDecelerate:)),
                 NSStringFromSelector(@selector(scrollViewDidEndDecelerating:)),
                 nil];
    
    NSTimeInterval startInterval = [[NSDate date] timeIntervalSince1970];
    
    for (int i = 0; i < 100000; i++) {
        for (NSString *selStr in selectors) {
            SEL sel = NSSelectorFromString(selStr);
            BOOL intercepted = isInterceptedSelector1(sel);
        }
    }
    
    NSTimeInterval endInterval = [[NSDate date] timeIntervalSince1970];
    NSLog(@"time consume is %f", endInterval - startInterval);

}

jessesquires · 2018-01-23T14:44:52Z

nice 😎

rnystrom

Awesome, thanks so much for running the test! I'd be happy to accept an update reordering to the most-used selectors.

facebook-github-bot · 2018-01-24T06:32:55Z

@zhongwuzw has updated the pull request. View: changes

zhongwuzw · 2018-01-24T06:44:09Z

@rnystrom , I found the main performance overhead is NSString compare for NSSet approach(I use std::unordered_set to store SEL pointer, time consumption can down to 0.4 based tests as before), secondly is Set, cause of hash computation(number of selectors is less, so use hash set is less than or compare), dispatch_once maybe not too big effect.

facebook-github-bot

@rnystrom is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Optimize SEL search efficiency

ffb18a9

facebook-github-bot added the CLA Signed label Jan 20, 2018

jessesquires approved these changes Jan 20, 2018

View reviewed changes

jessesquires added the ready for fb-import label Jan 20, 2018

Rename variable

1658f0b

rnystrom suggested changes Jan 23, 2018

View reviewed changes

reorder position of selectors

78b7a11

facebook-github-bot reviewed Jan 29, 2018

View reviewed changes

facebook-github-bot closed this in 8bccac9 Jan 30, 2018

zhongwuzw mentioned this pull request Aug 8, 2018

Make download receive response notification only dispatch to specific observer SDWebImage/SDWebImage#2426

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize SEL search efficiency #1055

Optimize SEL search efficiency #1055

zhongwuzw commented Jan 20, 2018 •

edited

Loading

facebook-github-bot commented Jan 20, 2018

facebook-github-bot commented Jan 20, 2018

jessesquires left a comment

jessesquires Jan 20, 2018

jessesquires Jan 20, 2018

facebook-github-bot commented Jan 21, 2018

iglistkit-bot commented Jan 21, 2018 •

edited

Loading

rnystrom commented Jan 22, 2018

zhongwuzw commented Jan 23, 2018

jessesquires commented Jan 23, 2018

rnystrom left a comment

facebook-github-bot commented Jan 24, 2018

zhongwuzw commented Jan 24, 2018 •

edited

Loading

facebook-github-bot left a comment

Optimize SEL search efficiency #1055

Optimize SEL search efficiency #1055

Conversation

zhongwuzw commented Jan 20, 2018 • edited Loading

Changes in this pull request

Checklist

facebook-github-bot commented Jan 20, 2018

facebook-github-bot commented Jan 20, 2018

jessesquires left a comment

Choose a reason for hiding this comment

jessesquires Jan 20, 2018

Choose a reason for hiding this comment

jessesquires Jan 20, 2018

Choose a reason for hiding this comment

facebook-github-bot commented Jan 21, 2018

iglistkit-bot commented Jan 21, 2018 • edited Loading

rnystrom commented Jan 22, 2018

zhongwuzw commented Jan 23, 2018

jessesquires commented Jan 23, 2018

rnystrom left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 24, 2018

zhongwuzw commented Jan 24, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

zhongwuzw commented Jan 20, 2018 •

edited

Loading

iglistkit-bot commented Jan 21, 2018 •

edited

Loading

zhongwuzw commented Jan 24, 2018 •

edited

Loading