From 06f36e236b6bde74cb2190b246a638d579a608ac Mon Sep 17 00:00:00 2001 From: Barijaona Ramaholimihaso Date: Fri, 1 Nov 2024 04:31:51 +0300 Subject: [PATCH 1/2] Reorganize @synchronized blocks in Folder Some of the former @synchronized occasionally led to infinite locks, so I reviewed to just use @synchronized for cache updates. --- Vienna/Sources/Models/Folder.m | 70 +++++++++++++++------------------- 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/Vienna/Sources/Models/Folder.m b/Vienna/Sources/Models/Folder.m index fd933995c..01737b020 100644 --- a/Vienna/Sources/Models/Folder.m +++ b/Vienna/Sources/Models/Folder.m @@ -430,9 +430,7 @@ -(void)clearFlag:(VNAFolderFlag)flagToClear */ -(void)setNonPersistedFlag:(VNAFolderFlag)flagToSet { - @synchronized(self) { - nonPersistedFlags |= flagToSet; - } + nonPersistedFlags |= flagToSet; } /* clearNonPersistedFlag @@ -440,9 +438,7 @@ -(void)setNonPersistedFlag:(VNAFolderFlag)flagToSet */ -(void)clearNonPersistedFlag:(VNAFolderFlag)flagToClear { - @synchronized(self) { - nonPersistedFlags &= ~flagToClear; - } + nonPersistedFlags &= ~flagToClear; } /* indexOfArticle @@ -450,20 +446,16 @@ -(void)clearNonPersistedFlag:(VNAFolderFlag)flagToClear */ -(NSUInteger)indexOfArticle:(Article *)article { - @synchronized(self) { - [self ensureCache]; - return [self.cachedGuids indexOfObject:article.guid]; - } + [self ensureCache]; + return [self.cachedGuids indexOfObject:article.guid]; } /* articleFromGuid */ -(Article *)articleFromGuid:(NSString *)guid { - @synchronized(self) { - [self ensureCache]; - return [self.cachedArticles objectForKey:guid]; - } + [self ensureCache]; + return [self.cachedArticles objectForKey:guid]; } /* retrieveKnownStatusForGuid @@ -488,10 +480,10 @@ -(NSInteger)retrieveKnownStatusForGuid:(NSString *)guid */ -(BOOL)createArticle:(Article *)article guidHistory:(NSArray *)guidHistory { -@synchronized(self) { // Prime the article cache [self ensureCache]; + @synchronized(self) { // Unread count adjustment factor NSInteger adjustment = 0; @@ -551,8 +543,8 @@ -(BOOL)createArticle:(Article *)article guidHistory:(NSArray *)guidHistory if (adjustment != 0) { [[Database sharedManager] setFolderUnreadCount:self adjustment:adjustment]; } + } // synchronized return YES; - } // synchronized } /* setUnreadCount @@ -616,6 +608,7 @@ -(void)ensureCache { NSAssert(self.type == VNAFolderTypeRSS || self.type == VNAFolderTypeOpenReader, @"Attempting to create cache for non RSS folder"); if (!self.isCached) { + @synchronized(self) { NSArray * myArray = [[Database sharedManager] minimalCacheForFolder:self.itemId]; for (Article * myArticle in myArray) { [myArticle beginContentAccess]; @@ -630,6 +623,7 @@ -(void)ensureCache self.isCached = YES; // Note that this only builds a minimal cache, so we cannot set the containsBodies flag // Note also that articles' statuses are left at the default value (0) which is ArticleStatusEmpty + } } } @@ -646,7 +640,6 @@ -(void)ensureCache */ -(void)markArticlesInCacheRead { -@synchronized(self) { NSInteger count = unreadCount; // Note the use of reverseObjectEnumerator // since the unread articles are likely to be clustered @@ -662,7 +655,6 @@ -(void)markArticlesInCacheRead } } } - } // synchronized } /* resetArticleStatuses @@ -696,7 +688,6 @@ -(void)resetArticleStatuses */ -(NSArray *)arrayOfUnreadArticlesRefs { -@synchronized(self) { if (self.isCached) { NSInteger count = unreadCount; NSMutableArray * result = [NSMutableArray arrayWithCapacity:unreadCount]; @@ -714,7 +705,6 @@ -(void)resetArticleStatuses } else { return [[Database sharedManager] arrayOfUnreadArticlesRefs:self.itemId]; } - } // synchronized } /*! Get an array of filtered articles in the current @@ -732,29 +722,29 @@ -(void)resetArticleStatuses } return [articles copy]; } - @synchronized(self) { - if (self.isCached && self.containsBodies) { - // attempt to retrieve from cache - NSMutableArray * articles = [NSMutableArray arrayWithCapacity:self.cachedGuids.count]; - for (id object in self.cachedGuids) { - Article * theArticle = [self.cachedArticles objectForKey:object]; - if (theArticle != nil) { - // deleted articles are not removed from cache any more - if (!theArticle.isDeleted) { - [articles addObject:theArticle]; - } - } else { - // some problem - NSLog(@"Bug retrieving from cache in folder %li : after %lu insertions of %lu, guid %@",(long)self.itemId, (unsigned long)articles.count,(unsigned long)self.cachedGuids.count,object); + if (self.isCached && self.containsBodies) { + // attempt to retrieve from cache + NSMutableArray * articles = [NSMutableArray arrayWithCapacity:self.cachedGuids.count]; + for (id object in self.cachedGuids) { + Article * theArticle = [self.cachedArticles objectForKey:object]; + if (theArticle != nil) { + // deleted articles are not removed from cache any more + if (!theArticle.isDeleted) { + [articles addObject:theArticle]; + } + } else { + // some problem + NSLog(@"Bug retrieving from cache in folder %li : after %lu insertions of %lu, guid %@",(long)self.itemId, (unsigned long)articles.count,(unsigned long)self.cachedGuids.count,object); + @synchronized(self) { [self.cachedArticles removeAllObjects]; return [self getCompleteArticles]; } } - return [articles copy]; - } else { - return [self getCompleteArticles]; - } - } // synchronized + } + return [articles copy]; + } else { + return [self getCompleteArticles]; + } } else { return [[Database sharedManager] arrayOfArticles:self.itemId filterString:filterString]; } @@ -767,6 +757,7 @@ -(void)resetArticleStatuses */ -(NSArray
*)getCompleteArticles { + @synchronized(self){ NSArray * articles = [[Database sharedManager] arrayOfArticles:self.itemId filterString:@""]; self.isCached = NO; self.containsBodies = NO; @@ -786,6 +777,7 @@ -(void)resetArticleStatuses self.containsBodies = YES; } return articles; + } } /* folderNameCompare From 1ca30a9a611058079cd11cf26c3d32c529dc39fa Mon Sep 17 00:00:00 2001 From: Barijaona Ramaholimihaso Date: Tue, 3 Dec 2024 11:15:05 +0300 Subject: [PATCH 2/2] Remove NSDiscardable protocol for Article I've come to the conclusion that we'd better off rolling back commit c520d75 which went into production with Vienna 3.9.3, as this has led to some unpredictable behavior while the resulting memory gain is not obvious and not worth the complication. --- Vienna/Sources/Database/Database.m | 6 ---- .../Sources/Main window/ArticleController.m | 6 ---- Vienna/Sources/Main window/ArticleConverter.m | 2 -- Vienna/Sources/Models/Article.h | 5 ++-- Vienna/Sources/Models/Article.m | 29 ------------------- Vienna/Sources/Models/Folder.m | 13 --------- 6 files changed, 2 insertions(+), 59 deletions(-) diff --git a/Vienna/Sources/Database/Database.m b/Vienna/Sources/Database/Database.m index 8eceba75b..d9d30583e 100644 --- a/Vienna/Sources/Database/Database.m +++ b/Vienna/Sources/Database/Database.m @@ -2331,7 +2331,6 @@ -(void)markArticleRead:(NSInteger)folderId guid:(NSString *)guid isRead:(BOOL)is Folder * folder = [self folderFromID:folderId]; if (folder != nil) { Article * article = [folder articleFromGuid:guid]; - [article beginContentAccess]; if (article != nil && isRead != article.read) { // Mark an individual article read FMDatabaseQueue *queue = self.databaseQueue; @@ -2347,7 +2346,6 @@ -(void)markArticleRead:(NSInteger)folderId guid:(NSString *)guid isRead:(BOOL)is [self setFolderUnreadCount:folder adjustment:adjustment]; } } - [article endContentAccess]; } } @@ -2428,7 +2426,6 @@ -(void)markArticleFlagged:(NSInteger)folderId guid:(NSString *)guid isFlagged:(B Folder * folder = [self folderFromID:folderId]; if (folder != nil) { Article * article = [folder articleFromGuid:guid]; - [article beginContentAccess]; if (article != nil && isFlagged != article.flagged) { FMDatabaseQueue *queue = self.databaseQueue; __block BOOL success; @@ -2444,7 +2441,6 @@ -(void)markArticleFlagged:(NSInteger)folderId guid:(NSString *)guid isFlagged:(B [article markFlagged:isFlagged]; } } - [article endContentAccess]; } } @@ -2474,9 +2470,7 @@ -(void)markArticleDeleted:(Article *)article isDeleted:(BOOL)isDeleted if (folder.countOfCachedArticles > 0) { // If we're in a smart folder, the cached article may be different. Article * cachedArticle = [folder articleFromGuid:guid]; - [cachedArticle beginContentAccess]; [cachedArticle markDeleted:isDeleted]; - [cachedArticle endContentAccess]; } } } diff --git a/Vienna/Sources/Main window/ArticleController.m b/Vienna/Sources/Main window/ArticleController.m index 0e6335344..cb307865b 100644 --- a/Vienna/Sources/Main window/ArticleController.m +++ b/Vienna/Sources/Main window/ArticleController.m @@ -530,7 +530,6 @@ -(void)refilterArrayOfArticles NSInteger filterMode = [Preferences standardPreferences].filterMode; for (NSInteger index = filteredArray.count - 1; index >= 0; --index) { Article * article = filteredArray[index]; - [article beginContentAccess]; if (guidOfArticleToPreserve != nil && article.folderId == articleToPreserve.folderId && [article.guid isEqualToString:guidOfArticleToPreserve]) { @@ -538,7 +537,6 @@ -(void)refilterArrayOfArticles } else if ([self filterArticle:article usingMode:filterMode] == false) { [filteredArray removeObjectAtIndex:index]; } - [article endContentAccess]; } if (guidOfArticleToPreserve != nil) { @@ -752,7 +750,6 @@ -(void)markFlaggedByArray:(NSArray *)articleArray flagged:(BOOL)flagged -(void)innerMarkFlaggedByArray:(NSArray *)articleArray flagged:(BOOL)flagged { for (Article * theArticle in articleArray) { - [theArticle beginContentAccess]; Folder *myFolder = [[Database sharedManager] folderFromID:theArticle.folderId]; if (myFolder.type == VNAFolderTypeOpenReader) { [[OpenReader sharedManager] markStarred:theArticle starredFlag:flagged]; @@ -761,7 +758,6 @@ -(void)innerMarkFlaggedByArray:(NSArray *)articleArray flagged:(BOOL)flagged guid:theArticle.guid isFlagged:flagged]; [theArticle markFlagged:flagged]; - [theArticle endContentAccess]; } } @@ -807,7 +803,6 @@ -(void)markReadByArray:(NSArray *)articleArray readFlag:(BOOL)readFlag -(void)innerMarkReadByArray:(NSArray *)articleArray readFlag:(BOOL)readFlag { for (Article * theArticle in articleArray) { - [theArticle beginContentAccess]; NSInteger folderId = theArticle.folderId; if (theArticle.read != readFlag) { if ([[Database sharedManager] folderFromID:folderId].type == VNAFolderTypeOpenReader) { @@ -817,7 +812,6 @@ -(void)innerMarkReadByArray:(NSArray *)articleArray readFlag:(BOOL)readFlag [theArticle markRead:readFlag]; } } - [theArticle endContentAccess]; } } diff --git a/Vienna/Sources/Main window/ArticleConverter.m b/Vienna/Sources/Main window/ArticleConverter.m index 29d24da3f..9f34a2de1 100644 --- a/Vienna/Sources/Main window/ArticleConverter.m +++ b/Vienna/Sources/Main window/ArticleConverter.m @@ -42,8 +42,6 @@ - (instancetype)init */ -(NSString *)expandTagsOfArticle:(Article *)theArticle intoTemplate:(NSString *)theString withConditional:(BOOL)cond { - NSAssert(theArticle.status != ArticleStatusDiscarded, - @"Attempting to access %@ with discarded elements", theArticle); NSMutableString * newString = [NSMutableString stringWithString:SafeString(theString)]; NSUInteger tagStartIndex = 0; diff --git a/Vienna/Sources/Models/Article.h b/Vienna/Sources/Models/Article.h index 8d4bfd314..a5aef227f 100644 --- a/Vienna/Sources/Models/Article.h +++ b/Vienna/Sources/Models/Article.h @@ -69,11 +69,10 @@ typedef NS_ENUM(NSInteger, VNAArticleFieldTag) { typedef NS_ENUM(NSInteger, ArticleStatus) { ArticleStatusEmpty = 0, ArticleStatusNew, - ArticleStatusUpdated, - ArticleStatusDiscarded + ArticleStatusUpdated }; -@interface Article : NSObject +@interface Article : NSObject // Accessor functions -(instancetype _Nonnull)initWithGuid:(NSString * _Nonnull)theGuid /*NS_DESIGNATED_INITIALIZER*/; diff --git a/Vienna/Sources/Models/Article.m b/Vienna/Sources/Models/Article.m index 031b5e8ac..a48f0dc31 100644 --- a/Vienna/Sources/Models/Article.m +++ b/Vienna/Sources/Models/Article.m @@ -308,33 +308,4 @@ -(NSScriptObjectSpecifier *)objectSpecifier return nil; } -// MARK: - NSDiscardableContent - --(BOOL)beginContentAccess -{ - return self.status != ArticleStatusDiscarded; -} - -- (void)endContentAccess -{ - // do nothing special, - // as we are not attempting to retrieve discarded content by ourself - // and don't manage any access count -} - --(void)discardContentIfPossible -{ - self.status = ArticleStatusDiscarded; - [articleData removeObjectForKey:MA_Field_Text]; - [articleData removeObjectForKey:MA_Field_Summary]; - [articleData removeObjectForKey:MA_Field_Author]; - [articleData removeObjectForKey:MA_Field_LastUpdate]; - [articleData removeObjectForKey:MA_Field_PublicationDate]; -} - -- (BOOL)isContentDiscarded -{ - return self.status == ArticleStatusDiscarded; -} - @end diff --git a/Vienna/Sources/Models/Folder.m b/Vienna/Sources/Models/Folder.m index 01737b020..87bddc5cc 100644 --- a/Vienna/Sources/Models/Folder.m +++ b/Vienna/Sources/Models/Folder.m @@ -70,7 +70,6 @@ -(instancetype)initWithId:(NSInteger)newId parentId:(NSInteger)newIdParent name: _containsBodies = NO; _hasPassword = NO; _cachedArticles = [NSCache new]; - _cachedArticles.evictsObjectsWithDiscardedContent = NO; _cachedArticles.delegate = self; _cachedGuids = [NSMutableArray array]; _attributes = [NSMutableDictionary dictionary]; @@ -497,7 +496,6 @@ -(BOOL)createArticle:(Article *)article guidHistory:(NSArray *)guidHistory return NO; // Article has been deleted and removed from database, so ignore } else { // add the article as new - [article beginContentAccess]; BOOL success = [[Database sharedManager] addArticle:article toFolder:self.itemId]; if (success) { article.status = ArticleStatusNew; @@ -510,17 +508,14 @@ -(BOOL)createArticle:(Article *)article guidHistory:(NSArray *)guidHistory adjustment = 1; } } else { - [article endContentAccess]; return NO; } - [article endContentAccess]; } } else if (existingArticle.deleted) { return NO; } else if (![[Preferences standardPreferences] boolForKey:MAPref_CheckForUpdatedArticles]) { return NO; } else { - [existingArticle beginContentAccess]; BOOL success = [[Database sharedManager] updateArticle:existingArticle ofFolder:self.itemId withArticle:article]; if (success) { // Update folder unread count if necessary @@ -533,10 +528,8 @@ -(BOOL)createArticle:(Article *)article guidHistory:(NSArray *)guidHistory } latestFetchCount++; } else { - [existingArticle endContentAccess]; return NO; } - [existingArticle endContentAccess]; } // Fix unread count on parent folders and Database manager @@ -611,14 +604,12 @@ -(void)ensureCache @synchronized(self) { NSArray * myArray = [[Database sharedManager] minimalCacheForFolder:self.itemId]; for (Article * myArticle in myArray) { - [myArticle beginContentAccess]; NSString * guid = myArticle.guid; myArticle.status = [self retrieveKnownStatusForGuid:guid]; [self.cachedArticles setObject:myArticle forKey:guid]; if (![self.cachedGuids containsObject:guid]) { [self.cachedGuids addObject:guid]; } - [myArticle endContentAccess]; } self.isCached = YES; // Note that this only builds a minimal cache, so we cannot set the containsBodies flag @@ -671,9 +662,7 @@ -(void)resetArticleStatuses if (article && (article.status == ArticleStatusNew || article.status == ArticleStatusUpdated)) { - [article beginContentAccess]; article.status = ArticleStatusEmpty; - [article endContentAccess]; count--; if (count == 0) { break; @@ -766,12 +755,10 @@ -(void)resetArticleStatuses if (self.type == VNAFolderTypeRSS || self.type == VNAFolderTypeOpenReader) { [self.cachedGuids removeAllObjects]; for (Article * article in articles) { - [article beginContentAccess]; NSString * guid = article.guid; article.status = [self retrieveKnownStatusForGuid:guid]; [self.cachedArticles setObject:article forKey:guid]; [self.cachedGuids addObject:guid]; - [article endContentAccess]; } self.isCached = YES; self.containsBodies = YES;