Skip to content
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

Fix slow [factory registerDefinition:] method. #330

Merged
merged 2 commits into from
Mar 19, 2015

Conversation

warnyul
Copy link
Contributor

@warnyul warnyul commented Mar 19, 2015

I described my solution here: #325

jasperblues added a commit that referenced this pull request Mar 19, 2015
Fix slow [factory registerDefinition:] method.
@jasperblues jasperblues merged commit 1c98abd into appsquickly:master Mar 19, 2015
@jasperblues
Copy link
Member

Thanks @warnyul !! Great work

@pawel-sekara
Copy link

It now crashes on iOS7 due to usage of NSString containsString: method

[https://github.com/appsquickly/Typhoon/blame/master/Source/Utils/TyphoonIntrospectionUtils.m#L151]

@warnyul
Copy link
Contributor Author

warnyul commented Mar 24, 2015

I'm sorry I did not notice. I fixed it and created a new pull request.

@alexgarbarev
Copy link
Contributor

@warnyul

// Little bit faster than protocol_isEqual method.

Where is proof about that? As I understand, protocol_isEqual is just pointer comparation instead of string full text search.. isn't true?

Update:
I expecting to see something like that: https://github.com/gnustep/libobjc2/blob/master/protocol.c#L461
So, pointer comparation at first, then string comparation (which is faster than fulltext search, because when first symbols are different - no need to scan the rest of the string)

P.S. I don't believe that we have objective reason to write protocol comparison faster than Apple did.

@alexgarbarev
Copy link
Contributor

@warnyul

+ (NSSet *)propertiesForClass:(Class)clazz upToParentClass:(Class)parent
{
    NSString *key = NSStringFromClass(clazz);

    if (propertiesCache == nil) {
        propertiesCache = [NSMutableDictionary dictionary];
    }
    else {
        if ([[propertiesCache allKeys] containsObject:key]) {
            return [propertiesCache objectForKey:key];
        }
    }

Is cache really needed there? As I understand, objective-c runtime does caching automatically when you introspect same class few times (then this cache release automatically by system).

Another problem here is same cache for different input arguments, because cache key isn't related on parent argument.

For example this equation always true in current code.

[TyphoonIntrospectionUtils propertiesForClass:[Assembly class] upToParentClass:[AssemblyParent class]] 
==
[TyphoonIntrospectionUtils propertiesForClass:[Assembly class] upToParentClass:[NSObject class]]

@alexgarbarev
Copy link
Contributor

@warnyul

- (void)_loadOnlyOne:(TyphoonDefinition *)definition
{
    [self preparePostProcessors];
    [_definitionPostProcessors enumerateObjectsUsingBlock:^(id<TyphoonDefinitionPostProcessor> postProcessor, NSUInteger idx, BOOL *stop) {
        [postProcessor postProcessDefinition:definition withFactory:self];
    }];
    [self newOrScopeCachedInstanceForDefinition:definition args:nil];
}

Calling line below is incorrect here. Because if definition scope is not singleton, this will produce new instance of definition when it's unexpectable.

[self newOrScopeCachedInstanceForDefinition:definition args:nil];

I faced with that bug in my current project. Prototype-scoped definition was created twice for one injection, but it was designed to be created once (network call at init time).

alexgarbarev added a commit that referenced this pull request May 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants