From 2f8290831a3a370ae2d677b4227a60e9e5589f1c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 14 Jun 2022 20:34:23 +0900 Subject: [PATCH] Skip quick import clause when importing a single item Closes https://github.com/ppy/osu/issues/18600. --- ...eneOnlinePlayBeatmapAvailabilityTracker.cs | 4 +- osu.Game/Beatmaps/BeatmapManager.cs | 2 +- osu.Game/Scoring/ScoreManager.cs | 2 +- osu.Game/Stores/RealmArchiveModelImporter.cs | 156 +++++++++--------- 4 files changed, 81 insertions(+), 83 deletions(-) diff --git a/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs b/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs index 278acce3e5..5cae576db1 100644 --- a/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs +++ b/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs @@ -225,10 +225,10 @@ namespace osu.Game.Tests.Online this.testBeatmapManager = testBeatmapManager; } - public override Live Import(BeatmapSetInfo item, ArchiveReader archive = null, CancellationToken cancellationToken = default) + public override Live Import(BeatmapSetInfo item, ArchiveReader archive = null, bool quickSkipIfExisting = false, CancellationToken cancellationToken = default) { testBeatmapManager.AllowImport.Task.WaitSafely(); - return (testBeatmapManager.CurrentImport = base.Import(item, archive, cancellationToken)); + return (testBeatmapManager.CurrentImport = base.Import(item, archive, quickSkipIfExisting, cancellationToken)); } } } diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 51f4d23bb4..ab614106cc 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -357,7 +357,7 @@ namespace osu.Game.Beatmaps public Task?> Import(ArchiveReader archive, bool lowPriority = false, CancellationToken cancellationToken = default) => beatmapModelManager.Import(archive, lowPriority, cancellationToken); - public Live? Import(BeatmapSetInfo item, ArchiveReader? archive = null, CancellationToken cancellationToken = default) => beatmapModelManager.Import(item, archive, cancellationToken); + public Live? Import(BeatmapSetInfo item, ArchiveReader? archive = null, CancellationToken cancellationToken = default) => beatmapModelManager.Import(item, archive, false, cancellationToken); public IEnumerable HandledExtensions => beatmapModelManager.HandledExtensions; diff --git a/osu.Game/Scoring/ScoreManager.cs b/osu.Game/Scoring/ScoreManager.cs index d42bf1b454..e086135296 100644 --- a/osu.Game/Scoring/ScoreManager.cs +++ b/osu.Game/Scoring/ScoreManager.cs @@ -280,7 +280,7 @@ namespace osu.Game.Scoring public Task>> Import(ProgressNotification notification, params ImportTask[] tasks) => scoreModelManager.Import(notification, tasks); - public Live Import(ScoreInfo item, ArchiveReader archive = null, bool lowPriority = false, CancellationToken cancellationToken = default) => scoreModelManager.Import(item, archive, cancellationToken); + public Live Import(ScoreInfo item, ArchiveReader archive = null, bool lowPriority = false, CancellationToken cancellationToken = default) => scoreModelManager.Import(item, archive, lowPriority, cancellationToken); public bool IsAvailableLocally(ScoreInfo model) => scoreModelManager.IsAvailableLocally(model); diff --git a/osu.Game/Stores/RealmArchiveModelImporter.cs b/osu.Game/Stores/RealmArchiveModelImporter.cs index 82b77bfb05..ac8a38fe37 100644 --- a/osu.Game/Stores/RealmArchiveModelImporter.cs +++ b/osu.Game/Stores/RealmArchiveModelImporter.cs @@ -235,7 +235,7 @@ namespace osu.Game.Stores return null; } - var scheduledImport = Task.Factory.StartNew(() => Import(model, archive, cancellationToken), + var scheduledImport = Task.Factory.StartNew(() => Import(model, archive, lowPriority, cancellationToken), cancellationToken, TaskCreationOptions.HideScheduler, lowPriority ? import_scheduler_low_priority : import_scheduler); @@ -248,106 +248,104 @@ namespace osu.Game.Stores /// /// The model to be imported. /// An optional archive to use for model population. + /// If true, imports will be skipped before they begin, given an existing model matches on hash and filenames. Should generally only be used for large batch imports, as it may defy user expectations when updating an existing model. /// An optional cancellation token. - public virtual Live? Import(TModel item, ArchiveReader? archive = null, CancellationToken cancellationToken = default) + public virtual Live? Import(TModel item, ArchiveReader? archive = null, bool quickSkipIfExisting = false, CancellationToken cancellationToken = default) => Realm.Run(realm => { - return Realm.Run(realm => + cancellationToken.ThrowIfCancellationRequested(); + + bool checkedExisting = false; + TModel? existing = null; + + if (quickSkipIfExisting && archive != null && !HasCustomHashFunction) { - cancellationToken.ThrowIfCancellationRequested(); + // this is a fast bail condition to improve large import performance. + item.Hash = computeHashFast(archive); - bool checkedExisting = false; - TModel? existing = null; + checkedExisting = true; + existing = CheckForExisting(item, realm); - if (archive != null && !HasCustomHashFunction) + if (existing != null) { - // this is a fast bail condition to improve large import performance. - item.Hash = computeHashFast(archive); + // bare minimum comparisons + // + // note that this should really be checking filesizes on disk (of existing files) for some degree of sanity. + // or alternatively doing a faster hash check. either of these require database changes and reprocessing of existing files. + if (CanSkipImport(existing, item) && + getFilenames(existing.Files).SequenceEqual(getShortenedFilenames(archive).Select(p => p.shortened).OrderBy(f => f)) && + checkAllFilesExist(existing)) + { + LogForModel(item, @$"Found existing (optimised) {HumanisedModelName} for {item} (ID {existing.ID}) – skipping import."); - checkedExisting = true; - existing = CheckForExisting(item, realm); + using (var transaction = realm.BeginWrite()) + { + UndeleteForReuse(existing); + transaction.Commit(); + } + + return existing.ToLive(Realm); + } + + LogForModel(item, @"Found existing (optimised) but failed pre-check."); + } + } + + try + { + LogForModel(item, @"Beginning import..."); + + // TODO: do we want to make the transaction this local? not 100% sure, will need further investigation. + using (var transaction = realm.BeginWrite()) + { + if (archive != null) + // TODO: look into rollback of file additions (or delayed commit). + item.Files.AddRange(createFileInfos(archive, Files, realm)); + + item.Hash = ComputeHash(item); + + // TODO: we may want to run this outside of the transaction. + Populate(item, archive, realm, cancellationToken); + + if (!checkedExisting) + existing = CheckForExisting(item, realm); if (existing != null) { - // bare minimum comparisons - // - // note that this should really be checking filesizes on disk (of existing files) for some degree of sanity. - // or alternatively doing a faster hash check. either of these require database changes and reprocessing of existing files. - if (CanSkipImport(existing, item) && - getFilenames(existing.Files).SequenceEqual(getShortenedFilenames(archive).Select(p => p.shortened).OrderBy(f => f)) && - checkAllFilesExist(existing)) + if (CanReuseExisting(existing, item)) { - LogForModel(item, @$"Found existing (optimised) {HumanisedModelName} for {item} (ID {existing.ID}) – skipping import."); + LogForModel(item, @$"Found existing {HumanisedModelName} for {item} (ID {existing.ID}) – skipping import."); - using (var transaction = realm.BeginWrite()) - { - UndeleteForReuse(existing); - transaction.Commit(); - } + UndeleteForReuse(existing); + transaction.Commit(); return existing.ToLive(Realm); } - LogForModel(item, @"Found existing (optimised) but failed pre-check."); - } - } + LogForModel(item, @"Found existing but failed re-use check."); - try - { - LogForModel(item, @"Beginning import..."); - - // TODO: do we want to make the transaction this local? not 100% sure, will need further investigation. - using (var transaction = realm.BeginWrite()) - { - if (archive != null) - // TODO: look into rollback of file additions (or delayed commit). - item.Files.AddRange(createFileInfos(archive, Files, realm)); - - item.Hash = ComputeHash(item); - - // TODO: we may want to run this outside of the transaction. - Populate(item, archive, realm, cancellationToken); - - if (!checkedExisting) - existing = CheckForExisting(item, realm); - - if (existing != null) - { - if (CanReuseExisting(existing, item)) - { - LogForModel(item, @$"Found existing {HumanisedModelName} for {item} (ID {existing.ID}) – skipping import."); - - UndeleteForReuse(existing); - transaction.Commit(); - - return existing.ToLive(Realm); - } - - LogForModel(item, @"Found existing but failed re-use check."); - - existing.DeletePending = true; - } - - PreImport(item, realm); - - // import to store - realm.Add(item); - - transaction.Commit(); + existing.DeletePending = true; } - LogForModel(item, @"Import successfully completed!"); - } - catch (Exception e) - { - if (!(e is TaskCanceledException)) - LogForModel(item, @"Database import or population failed and has been rolled back.", e); + PreImport(item, realm); - throw; + // import to store + realm.Add(item); + + transaction.Commit(); } - return (Live?)item.ToLive(Realm); - }); - } + LogForModel(item, @"Import successfully completed!"); + } + catch (Exception e) + { + if (!(e is TaskCanceledException)) + LogForModel(item, @"Database import or population failed and has been rolled back.", e); + + throw; + } + + return (Live?)item.ToLive(Realm); + }); /// /// Any file extensions which should be included in hash creation.