From f3128b562dd5e270bedcf5c5e38eaba82576ca2a Mon Sep 17 00:00:00 2001 From: Robert McRackan Date: Mon, 18 Nov 2019 14:37:17 -0500 Subject: [PATCH] Fix performance issues, esp regarding saving tags --- .gitignore | 4 ++ .../UNTESTED/LibraryCommands.cs | 8 +-- .../UNTESTED/SearchEngineCommands.cs | 3 +- DataLayer/UNTESTED/EfClasses/Book.cs | 15 ++-- .../UNTESTED/EfClasses/UserDefinedItem.cs | 41 ++++++----- .../UNTESTED/QueryObjects/LibraryQueries.cs | 14 ++-- .../UNTESTED/TagPersistenceInterceptor.cs | 66 ++++++------------ DtoImporterService/BookImporter.cs | 44 ++++++------ FileManager/FileManager.csproj | 4 ++ FileManager/UNTESTED/TagsPersistence.cs | 69 +++++++++---------- InternalUtilities/InternalUtilities.csproj | 4 -- LibationWinForm/UNTESTED/ProductsGrid.cs | 46 ++++--------- 12 files changed, 142 insertions(+), 176 deletions(-) diff --git a/.gitignore b/.gitignore index 3e759b75..fc97da24 100644 --- a/.gitignore +++ b/.gitignore @@ -328,3 +328,7 @@ ASALocalRun/ # MFractors (Xamarin productivity tool) working folder .mfractor/ + + +# manually ignored files +/__TODO.txt diff --git a/ApplicationServices/UNTESTED/LibraryCommands.cs b/ApplicationServices/UNTESTED/LibraryCommands.cs index 5aa52f85..aed5f5b9 100644 --- a/ApplicationServices/UNTESTED/LibraryCommands.cs +++ b/ApplicationServices/UNTESTED/LibraryCommands.cs @@ -23,14 +23,12 @@ namespace ApplicationServices return (totalCount, newCount); } - public static int IndexChangedTags(Book book) + public static int UpdateTags(this LibationContext context, Book book, string newTags) { - // update disconnected entity - using var context = LibationContext.Create(); - context.Update(book); + book.UserDefinedItem.Tags = newTags; + var qtyChanges = context.SaveChanges(); - // this part is tags-specific if (qtyChanges > 0) SearchEngineCommands.UpdateBookTags(book); diff --git a/ApplicationServices/UNTESTED/SearchEngineCommands.cs b/ApplicationServices/UNTESTED/SearchEngineCommands.cs index 63e5d4a9..b1dccedb 100644 --- a/ApplicationServices/UNTESTED/SearchEngineCommands.cs +++ b/ApplicationServices/UNTESTED/SearchEngineCommands.cs @@ -1,5 +1,4 @@ -using System.Threading.Tasks; -using DataLayer; +using DataLayer; using LibationSearchEngine; namespace ApplicationServices diff --git a/DataLayer/UNTESTED/EfClasses/Book.cs b/DataLayer/UNTESTED/EfClasses/Book.cs index cb3d69eb..9c83a9af 100644 --- a/DataLayer/UNTESTED/EfClasses/Book.cs +++ b/DataLayer/UNTESTED/EfClasses/Book.cs @@ -61,12 +61,17 @@ namespace DataLayer string title, string description, int lengthInMinutes, - IEnumerable authors) + IEnumerable authors, + IEnumerable narrators) { // validate ArgumentValidator.EnsureNotNull(audibleProductId, nameof(audibleProductId)); var productId = audibleProductId.Id; ArgumentValidator.EnsureNotNullOrWhiteSpace(productId, nameof(productId)); + + // assign as soon as possible. stuff below relies on this + AudibleProductId = productId; + ArgumentValidator.EnsureNotNullOrWhiteSpace(title, nameof(title)); // non-ef-ctor init.s @@ -79,19 +84,13 @@ namespace DataLayer CategoryId = Category.GetEmpty().CategoryId; // simple assigns - AudibleProductId = productId; Title = title; Description = description; LengthInMinutes = lengthInMinutes; // assigns with biz logic ReplaceAuthors(authors); - //ReplaceNarrators(narrators); - - // import previously saved tags - // do this immediately. any save occurs before reloading tags will overwrite persistent tags with new blank entries; all old persisted tags will be lost - // if refactoring, DO NOT use "ProductId" before it's assigned to. to be safe, just use "productId" - UserDefinedItem = new UserDefinedItem(this) { Tags = FileManager.TagsPersistence.GetTags(productId) }; + ReplaceNarrators(narrators); } #region contributors, authors, narrators diff --git a/DataLayer/UNTESTED/EfClasses/UserDefinedItem.cs b/DataLayer/UNTESTED/EfClasses/UserDefinedItem.cs index 0cb292e6..244f64b2 100644 --- a/DataLayer/UNTESTED/EfClasses/UserDefinedItem.cs +++ b/DataLayer/UNTESTED/EfClasses/UserDefinedItem.cs @@ -13,29 +13,36 @@ namespace DataLayer private UserDefinedItem() { } internal UserDefinedItem(Book book) - { - ArgumentValidator.EnsureNotNull(book, nameof(book)); + { + ArgumentValidator.EnsureNotNull(book, nameof(book)); Book = book; - } + + // import previously saved tags + ArgumentValidator.EnsureNotNullOrWhiteSpace(book.AudibleProductId, nameof(book.AudibleProductId)); + Tags = FileManager.TagsPersistence.GetTags(book.AudibleProductId); + } private string _tags = ""; public string Tags { get => _tags; set => _tags = sanitize(value); - } - #region sanitize tags: space delimited. Inline/denormalized. Lower case. Alpha numeric and hyphen - // only legal chars are letters numbers underscores and separating whitespace - // - // technically, the only char.s which aren't easily supported are \ [ ] - // however, whitelisting is far safer than blacklisting (eg: new lines, non-printable character) - // it's easy to expand whitelist as needed - // for lucene, ToLower() isn't needed because search is case-inspecific. for here, it prevents duplicates - // - // there are also other allowed but misleading characters. eg: the ^ operator defines a 'boost' score - // full list of characters which must be escaped: - // + - && || ! ( ) { } [ ] ^ " ~ * ? : \ - static Regex regex = new Regex(@"[^\w\d\s_]", RegexOptions.Compiled); + } + + public IEnumerable TagsEnumerated => Tags == "" ? new string[0] : Tags.Split(null as char[], StringSplitOptions.RemoveEmptyEntries); + + #region sanitize tags: space delimited. Inline/denormalized. Lower case. Alpha numeric and hyphen + // only legal chars are letters numbers underscores and separating whitespace + // + // technically, the only char.s which aren't easily supported are \ [ ] + // however, whitelisting is far safer than blacklisting (eg: new lines, non-printable character) + // it's easy to expand whitelist as needed + // for lucene, ToLower() isn't needed because search is case-inspecific. for here, it prevents duplicates + // + // there are also other allowed but misleading characters. eg: the ^ operator defines a 'boost' score + // full list of characters which must be escaped: + // + - && || ! ( ) { } [ ] ^ " ~ * ? : \ + static Regex regex { get; } = new Regex(@"[^\w\d\s_]", RegexOptions.Compiled); private static string sanitize(string input) { if (string.IsNullOrWhiteSpace(input)) @@ -63,8 +70,6 @@ namespace DataLayer return string.Join(" ", unique); } - - public IEnumerable TagsEnumerated => Tags == "" ? new string[0] : Tags.Split(null as char[], StringSplitOptions.RemoveEmptyEntries); #endregion // owned: not an optional one-to-one diff --git a/DataLayer/UNTESTED/QueryObjects/LibraryQueries.cs b/DataLayer/UNTESTED/QueryObjects/LibraryQueries.cs index cef556e2..ecd3700e 100644 --- a/DataLayer/UNTESTED/QueryObjects/LibraryQueries.cs +++ b/DataLayer/UNTESTED/QueryObjects/LibraryQueries.cs @@ -5,13 +5,19 @@ using Microsoft.EntityFrameworkCore; namespace DataLayer { public static class LibraryQueries - { - public static List GetLibrary_Flat_NoTracking() + { + public static List GetLibrary_Flat_WithTracking(this LibationContext context) + => context + .Library + .GetLibrary() + .ToList(); + + public static List GetLibrary_Flat_NoTracking() { using var context = LibationContext.Create(); return context .Library -//.AsNoTracking() + .AsNoTracking() .GetLibrary() .ToList(); } @@ -21,7 +27,7 @@ namespace DataLayer using var context = LibationContext.Create(); return context .Library -//.AsNoTracking() + .AsNoTracking() .GetLibraryBook(productId); } diff --git a/DataLayer/UNTESTED/TagPersistenceInterceptor.cs b/DataLayer/UNTESTED/TagPersistenceInterceptor.cs index 7a83fc08..0d4bc606 100644 --- a/DataLayer/UNTESTED/TagPersistenceInterceptor.cs +++ b/DataLayer/UNTESTED/TagPersistenceInterceptor.cs @@ -4,57 +4,35 @@ using System.Linq; using Dinah.Core.Collections.Generic; using Dinah.EntityFrameworkCore; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.ChangeTracking; namespace DataLayer { internal class TagPersistenceInterceptor : IDbInterceptor { - public void Executing(DbContext context) - { - doWork__EFCore(context); - } - public void Executed(DbContext context) { } - static void doWork__EFCore(DbContext context) - { - // persist tags: - var modifiedEntities = context.ChangeTracker.Entries().Where(p => p.State.In(EntityState.Modified, EntityState.Added)).ToList(); - var tagSets = modifiedEntities.Select(e => e.Entity as UserDefinedItem).Where(a => a != null).ToList(); - foreach (var t in tagSets) - FileManager.TagsPersistence.Save(t.Book.AudibleProductId, t.Tags); - } + public void Executing(DbContext context) + { + // persist tags: + var modifiedEntities = context + .ChangeTracker + .Entries() + .Where(p => p.State.In(EntityState.Modified, EntityState.Added)) + .ToList(); - #region // notes: working with proxies, esp EF 6 - // EF 6: entities are proxied with lazy loading when collections are virtual - // EF Core: lazy loading is supported in 2.1 (there is a version of lazy loading with proxy-wrapping and a proxy-less version with DI) but not on by default and are not supported here + persistTags(modifiedEntities); + } - //static void doWork_EF6(DbContext context) - //{ - // var modifiedEntities = context.ChangeTracker.Entries().Where(p => p.State == EntityState.Modified).ToList(); - // var unproxiedEntities = modifiedEntities.Select(me => UnProxy(context, me.Entity)).ToList(); - - // // persist tags - // var tagSets = unproxiedEntities.Select(ue => ue as UserDefinedItem).Where(a => a != null).ToList(); - // foreach (var t in tagSets) - // FileManager.TagsPersistence.Save(t.ProductId, t.TagsRaw); - //} - - //// https://stackoverflow.com/a/25774651 - //private static T UnProxy(DbContext context, T proxyObject) where T : class - //{ - // // alternative: https://docs.microsoft.com/en-us/ef/ef6/fundamentals/proxies - // var proxyCreationEnabled = context.Configuration.ProxyCreationEnabled; - // try - // { - // context.Configuration.ProxyCreationEnabled = false; - // return context.Entry(proxyObject).CurrentValues.ToObject() as T; - // } - // finally - // { - // context.Configuration.ProxyCreationEnabled = proxyCreationEnabled; - // } - //} - #endregion - } + private static void persistTags(List modifiedEntities) + { + var tagSets = modifiedEntities + .Select(e => e.Entity as UserDefinedItem) + // filter by null but NOT by blank. blank is the valid way to show the absence of tags + .Where(a => a != null) + .ToList(); + foreach (var t in tagSets) + FileManager.TagsPersistence.Save(t.Book.AudibleProductId, t.Tags); + } + } } diff --git a/DtoImporterService/BookImporter.cs b/DtoImporterService/BookImporter.cs index bf24b409..8f59c837 100644 --- a/DtoImporterService/BookImporter.cs +++ b/DtoImporterService/BookImporter.cs @@ -57,38 +57,40 @@ namespace DtoImporterService .Select(a => context.Contributors.Local.Single(c => a.Name == c.Name)) .ToList(); + // if no narrators listed, author is the narrator + if (item.Narrators is null || !item.Narrators.Any()) + item.Narrators = item.Authors; + // nested logic is required so order of names is retained. else, contributors may appear in the order they were inserted into the db + var narrators = item + .Narrators + .Select(n => context.Contributors.Local.Single(c => n.Name == c.Name)) + .ToList(); + book = context.Books.Add(new Book( - new AudibleProductId(item.ProductId), item.Title, item.Description, item.LengthInMinutes, authors)) - .Entity; + new AudibleProductId(item.ProductId), + item.Title, + item.Description, + item.LengthInMinutes, + authors, + narrators) + ).Entity; + + var publisherName = item.Publisher; + if (!string.IsNullOrWhiteSpace(publisherName)) + { + var publisher = context.Contributors.Local.Single(c => publisherName == c.Name); + book.ReplacePublisher(publisher); + } qtyNew++; } - // if no narrators listed, author is the narrator - if (item.Narrators is null || !item.Narrators.Any()) - item.Narrators = item.Authors; - // nested logic is required so order of names is retained. else, contributors may appear in the order they were inserted into the db - var narrators = item - .Narrators - .Select(n => context.Contributors.Local.Single(c => n.Name == c.Name)) - .ToList(); - // not all books have narrators. these will already be using author as narrator. don't undo this - if (narrators.Any()) - book.ReplaceNarrators(narrators); - // set/update book-specific info which may have changed book.PictureId = item.PictureId; book.UpdateProductRating(item.Product_OverallStars, item.Product_PerformanceStars, item.Product_StoryStars); if (!string.IsNullOrWhiteSpace(item.SupplementUrl)) book.AddSupplementDownloadUrl(item.SupplementUrl); - var publisherName = item.Publisher; - if (!string.IsNullOrWhiteSpace(publisherName)) - { - var publisher = context.Contributors.Local.Single(c => publisherName == c.Name); - book.ReplacePublisher(publisher); - } - // important to update user-specific info. this will have changed if user has rated/reviewed the book since last library import book.UserDefinedItem.UpdateRating(item.MyUserRating_Overall, item.MyUserRating_Performance, item.MyUserRating_Story); diff --git a/FileManager/FileManager.csproj b/FileManager/FileManager.csproj index 0ce9b091..9f2d09b8 100644 --- a/FileManager/FileManager.csproj +++ b/FileManager/FileManager.csproj @@ -4,6 +4,10 @@ netstandard2.1 + + + + diff --git a/FileManager/UNTESTED/TagsPersistence.cs b/FileManager/UNTESTED/TagsPersistence.cs index 90f5349a..58666f1e 100644 --- a/FileManager/UNTESTED/TagsPersistence.cs +++ b/FileManager/UNTESTED/TagsPersistence.cs @@ -3,64 +3,57 @@ using System.Collections.Generic; using System.IO; using System.Linq; using Newtonsoft.Json; +using Polly; +using Polly.Retry; namespace FileManager { /// /// Tags must also be stored in db for search performance. Stored in json file to survive a db reset. - /// json is only read when a product is first loaded + /// json is only read when a product is first loaded into the db /// json is only written to when tags are edited /// json access is infrequent and one-off - /// all other reads happen against db. No volitile storage /// public static class TagsPersistence { - public static string TagsFile => Path.Combine(Configuration.Instance.LibationFiles, "BookTags.json"); + private static string TagsFile => Path.Combine(Configuration.Instance.LibationFiles, "BookTags.json"); private static object locker { get; } = new object(); + // if failed, retry only 1 time after a wait of 100 ms + // 1st save attempt sometimes fails with + // The requested operation cannot be performed on a file with a user-mapped section open. + private static RetryPolicy policy { get; } + = Policy.Handle() + .WaitAndRetry(new[] { TimeSpan.FromMilliseconds(100) }); + public static void Save(string productId, string tags) - => System.Threading.Tasks.Task.Run(() => save_fireAndForget(productId, tags)); - - private static void save_fireAndForget(string productId, string tags) { + ensureCache(); + + cache[productId] = tags; + lock (locker) - { - // get all - var allDictionary = retrieve(); - - // update/upsert tag list - allDictionary[productId] = tags; - - // re-save: - // this often fails the first time with - // The requested operation cannot be performed on a file with a user-mapped section open. - // 2nd immediate attempt failing was rare. So I added sleep. We'll see... - void resave() => File.WriteAllText(TagsFile, JsonConvert.SerializeObject(allDictionary, Formatting.Indented)); - try { resave(); } - catch (IOException debugEx) - { - // 1000 was always reliable but very slow. trying other values - var waitMs = 100; - - System.Threading.Thread.Sleep(waitMs); - resave(); - } - } + policy.Execute(() => File.WriteAllText(TagsFile, JsonConvert.SerializeObject(cache, Formatting.Indented))); } + private static Dictionary cache; + public static string GetTags(string productId) { - var dic = retrieve(); - return dic.ContainsKey(productId) ? dic[productId] : null; + ensureCache(); + + cache.TryGetValue(productId, out string value); + return value; } - private static Dictionary retrieve() - { - if (!FileUtility.FileExists(TagsFile)) - return new Dictionary(); - lock (locker) - return JsonConvert.DeserializeObject>(File.ReadAllText(TagsFile)); - } - } + private static void ensureCache() + { + if (cache is null) + lock (locker) + cache = !FileUtility.FileExists(TagsFile) + ? new Dictionary() + : JsonConvert.DeserializeObject>(File.ReadAllText(TagsFile)); + } + } } diff --git a/InternalUtilities/InternalUtilities.csproj b/InternalUtilities/InternalUtilities.csproj index 7650ffef..531ac0e6 100644 --- a/InternalUtilities/InternalUtilities.csproj +++ b/InternalUtilities/InternalUtilities.csproj @@ -4,10 +4,6 @@ netstandard2.1 - - - - diff --git a/LibationWinForm/UNTESTED/ProductsGrid.cs b/LibationWinForm/UNTESTED/ProductsGrid.cs index 611fe881..1206fd29 100644 --- a/LibationWinForm/UNTESTED/ProductsGrid.cs +++ b/LibationWinForm/UNTESTED/ProductsGrid.cs @@ -27,10 +27,15 @@ namespace LibationWinForm public event EventHandler VisibleCountChanged; private DataGridView dataGridView; + private LibationContext context; - public ProductsGrid() => InitializeComponent(); + public ProductsGrid() + { + InitializeComponent(); + Disposed += (_, __) => { if (context != null) context.Dispose(); }; + } - private bool hasBeenDisplayed = false; + private bool hasBeenDisplayed = false; public void Display() { if (hasBeenDisplayed) @@ -87,10 +92,11 @@ namespace LibationWinForm } - // - // transform into sorted GridEntry.s BEFORE binding - // - var lib = LibraryQueries.GetLibrary_Flat_NoTracking(); + // + // transform into sorted GridEntry.s BEFORE binding + // + context = LibationContext.Create(); + var lib = context.GetLibrary_Flat_WithTracking(); // if no data. hide all columns. return if (!lib.Any()) @@ -174,8 +180,8 @@ namespace LibationWinForm if (editTagsForm.ShowDialog() != DialogResult.OK) return; - var qtyChanges = saveChangedTags(liveGridEntry.GetBook(), editTagsForm.NewTags); - if (qtyChanges == 0) + var qtyChanges = context.UpdateTags(liveGridEntry.GetBook(), editTagsForm.NewTags); + if (qtyChanges == 0) return; // force a re-draw, and re-apply filters @@ -186,14 +192,6 @@ namespace LibationWinForm filter(); } - private static int saveChangedTags(Book book, string newTags) - { - book.UserDefinedItem.Tags = newTags; - - var qtyChanges = LibraryCommands.IndexChangedTags(book); - return qtyChanges; - } - #region Cell Formatting private void replaceFormatted(object sender, DataGridViewCellFormattingEventArgs e) { @@ -213,22 +211,6 @@ namespace LibationWinForm } #endregion - public void UpdateRow(string productId) - { - for (var r = dataGridView.RowCount - 1; r >= 0; r--) - { - var gridEntry = getGridEntry(r); - if (gridEntry.GetBook().AudibleProductId == productId) - { - var libBook = LibraryQueries.GetLibraryBook_Flat_NoTracking(productId); - gridEntry.REPLACE_Library_Book(libBook); - dataGridView.InvalidateRow(r); - - return; - } - } - } - #region filter string _filterSearchString; private void filter() => Filter(_filterSearchString);