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

Add option to enable or disable weak memory cache for SDImageCache #2379

Merged
merged 1 commit into from Jul 11, 2018

Conversation

dreampiggy
Copy link
Contributor

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: #2377

Pull Request Description

Reason

In the previous #2228 , we introduce a secondary weak memory cache for SDImageCache. Which may solve #791 case that a UIImage instance is hold by other alive instance by strong reference, however, when we purge the memory cache, we can not investigate this and cause re-query disk cahce or re-download from network.

I think that feature's aim is correct and it actually solve this problem. However, this behavior should be controlled by user but not hidden in the implementation detail. Because a weak maptable which may delay intance dealloc time at race time (See this blog). And a extra lock to keep thread-safe which may a little impact the performance. So it's greate that user can decide to enable/disable this feature.

The default options is ON, since most of cases this works does not cause any problem.

@codecov-io
Copy link

codecov-io commented Jul 7, 2018

Codecov Report

Merging #2379 into master will decrease coverage by 0.04%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2379      +/-   ##
=========================================
- Coverage   75.44%   75.4%   -0.05%     
=========================================
  Files          36      36              
  Lines        3837    3847      +10     
=========================================
+ Hits         2895    2901       +6     
- Misses        942     946       +4
Impacted Files Coverage Δ
SDWebImage/SDImageCacheConfig.m 100% <100%> (ø) ⬆️
SDWebImage/SDImageCache.m 63.44% <63.63%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e98a941...bfa9403. Read the comment docs.

Copy link
Contributor

@mythodeia mythodeia left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bpoplauschi bpoplauschi left a comment

Choose a reason for hiding this comment

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

Looks good to me as well.
The only question: do you want to merge this into master and deliver another release 4.4.x? Or just add it to 5.0.0?

@bpoplauschi bpoplauschi added this to the 4.4.2 milestone Jul 11, 2018
@bpoplauschi
Copy link
Member

Ok, I just saw 4.4.2 release. Let's bring it there.

@bpoplauschi bpoplauschi merged commit f28796d into SDWebImage:master Jul 11, 2018
@bpoplauschi bpoplauschi deleted the feature_weak_cache_option branch July 11, 2018 10:14
@dreampiggy dreampiggy mentioned this pull request Jun 20, 2019
7 tasks
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.

None yet

4 participants