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.

Sign up for GitHub

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

Jump to bottom

don't let NSCache evict section controller cache objects as it causes an infinite feed load loop #775

Merged
merged 10 commits into from Jul 13, 2016
Merged

don't let NSCache evict section controller cache objects as it causes an infinite feed load loop #775

merged 10 commits into from Jul 13, 2016

Conversation

joewalsh
Copy link
Contributor

No description provided.

@joewalsh joewalsh changed the title don't let NSCache evict section controller cache objects, causes infinite feed load loop don't let NSCache evict section controller cache objects as it causes an infinite feed load loop Jul 12, 2016
@codecov-io
Copy link

codecov-io commented Jul 13, 2016 •
edited

Current coverage is 21.71%

Merging #775 into develop will decrease coverage by 0.10%

Powered by Codecov. Last updated by 8b52716...ce60313

@joewalsh
Copy link
Contributor Author

joewalsh commented Jul 13, 2016 •
edited

So here's what was happening:

If the user has more than totalMaxNumberOfSections, as the section controllers were all created, the section controller cache would evict the initial controllers that were added (which happened to be visible on screen). When WMFExploreViewController requested those visible sections from the cache, they were no longer available, so they were re-created and re-evicted, on and on forever.

Since WMFExploreViewController expects the visible sections/controllers to exist in the cache, I gave it control over what gets evicted from the cache and when. The WMFExploreSectionControllerCache can no longer self evict. Also, instead of only being able to evict everything from the cache, it evicts sections that aren't visible (which has the side effect of fixing the complete section reload on app exit & relaunch).

coreyfloyd
coreyfloyd reviewed Jul 13, 2016
View reviewed changes
Wikipedia/Code/WMFExploreSectionControllerCache.m
self.reverseLookup =
[NSMapTable mapTableWithKeyOptions:NSMapTableWeakMemory | NSMapTableObjectPointerPersonality
valueOptions:NSMapTableWeakMemory];
self.reverseLookup = [[NSMutableDictionary alloc] init];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer need this to be weak? Is it expected that the new behavior will take care of cleaning up all the objects in the reverse lookup?

Also, is it ok that the section controllers don't conform to NSCopying - will the dictionary be able to find them based on pointer equality?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind… you are now using the hash… so that we do't have to deal with the section controllers themselves

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New question… do we need to implement the hash method for the controllers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new expected behavior is that the view controller owns the cache and cleans it. The hash method on NSObject seems to suffice.

@coreyfloyd
Copy link
Contributor

👍

Waiting for travis… its being weird today

@coreyfloyd
Copy link
Contributor

Merging and will let jerkins do the testing

@coreyfloyd coreyfloyd merged commit 7bc3f7b into develop Jul 13, 2016
@coreyfloyd coreyfloyd deleted the hotfix/section_controller_cache branch July 13, 2016 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@joewalsh @codecov-io @coreyfloyd