Performance improvements #3808

JohnMcPMS · 2023-10-24T18:11:55Z

Change

This change has several performance improvements based on profiling.

Installed package caching

A global process cache is stored for the unfiltered install data. It is very basic for updates right now to make it more reasonable to service, but future changes could improve things to examine the system data and only do delta updates. It could also be saved to disk to improve the performance of future processes.

The cache itself is not used by any callers, but instead in-memory copies are handed out. The cache sets up event listeners for the current data sources and waits for these events before it will attempt to update again. A full cache hit requires only copying the memory database, which SQLite has an efficient method for.

The only performance gain to the update path that is provided by this implementation is to reuse the names of MSIX packages, but since this accounts for a huge amount of creating the index it results in ~50% time reduction in creating an updated installed source (without any MSIX changes).

ICU regex caching

The regular expressions that we use were being compiled repeatedly. Since this is a fixed set of expressions, they are all now cached and copies are used for actual operation.

Version lookup (string)

The version lookup by string (such as winget show FOO -v VER) was doing a full table scan. This change shifts to pulling all of the version for the given identifier out and using Version object comparisons to find the proper result. This has the side effect of fixing a somewhat annoying user interaction where trailing 0s in a version were still required to find from the command line. One can now provide the smaller version string and still find the expected package version (eg 1.2 will now find version 1.2.0).

Version lookup (available version)

The round trip from retrieving the available version information to getting the manifest has been improved by including the manifest id value directly in the available version data. The available version package object now caches the version string to manifest id data as well, meaning that any usage of a PackageVersionKey that was retrieved from GetAvailableVersions will directly return the package version object without querying the index.

Let property lookups handle an unknown manifest id

The GetProperty code was enforcing that the manifest id was valid and then reusing existing code that did not handle a missing manifest. Creating new methods that can handle the missing manifest allows us to remove the initial check.

Validation

Added tests where appropriate, regression covers many other cases.

Microsoft Reviewers: Open in CodeFlow

…ond wait

denelon · 2023-11-02T20:51:48Z

Related to:

Windows Package Manager Server runs on high CPU #3667

…pacts of events

florelis · 2023-11-07T18:24:34Z

src/AppInstallerCommonCore/Regex.cpp

@@ -20,6 +21,66 @@ namespace AppInstaller::Regex
        using uregex_ptr = wil::unique_any<URegularExpression*, decltype(uregex_close), uregex_close>;
        using utext_ptr = wil::unique_any<UText*, decltype(utext_close), utext_close>;

+        static std::unique_ptr<impl> Create(std::string_view pattern, Options options)


Nit: can you add a comment stating this does caching to avoid recompiling the regex, please?

florelis · 2023-11-07T18:29:44Z

src/AppInstallerCommonCore/Regex.cpp

+                auto itr = s_regex_cache.map.find(requested);
+                if (itr != s_regex_cache.map.end())
+                {
+                    return std::make_unique<impl>(itr->second);
+                }
+            }
+
+            auto exclusiveLock = s_regex_cache.lock.lock_exclusive();
+
+            auto itr = s_regex_cache.map.find(requested);
+            if (itr != s_regex_cache.map.end())
+            {
+                return std::make_unique<impl>(itr->second);


Nit: please add a comment about why we need to search again with the exclusive lock. or anything else to not have to figure out the concurrency stuff when reading the code

florelis · 2023-11-07T18:38:23Z

src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/ManifestTable.h

+        static std::optional<typename Table::id_t> GetIdById(const SQLite::Connection& connection, SQLite::rowid_t id)
+        {
+            auto statement = details::ManifestTableGetIdsById_Statement(connection, id, { details::GetManifestTableColumnName<Table>() }, false);
+            if (statement.Step()) { return statement.GetColumn<typename Table::id_t>(0); } else { return std::nullopt; }


nit: linebreaks

florelis · 2023-11-07T18:48:11Z

src/AppInstallerSharedLib/SQLiteStorageBase.cpp

@@ -40,6 +40,11 @@ namespace AppInstaller::SQLite
        return Utility::ConvertUnixEpochToSystemClock(lastWriteTime);
    }

+    std::string SQLiteStorageBase::GetDatabaseIdentifier()
+    {
+        return MetadataTable::GetNamedValue<std::string>(m_dbconn, s_MetadataValueName_DatabaseIdentifier);


Does this return empty when not set?

florelis · 2023-11-07T18:51:25Z

src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h

+        // Creating the source reference for this is sufficient to cause the cache to be updated on next Open.
+        InstalledForceCacheUpdate,


It's not clear to me if this is an actual source I could use, or if it is just to force the update. Maybe update the comment to "Same as Installed, but creating the source reference causes the cache to be updated on the next Open"

Edit: It was clear once I saw the usage, but I'd still appreciate a clarification here

florelis · 2023-11-07T19:21:22Z

src/AppInstallerCLIE2ETests/Interop/InstallInterop.cs

+            var installedCatalogReference = this.packageManager.GetLocalPackageCatalog(LocalPackageCatalog.InstalledPackages);
+
+            // Ensure package is not installed
+            string targetPackageProductCode = "{A499DD5E-8DC5-4AD2-911A-BCD0263295E9}";


Nit: I had to look up what this GUID was. Can this be a constant somewhere else?

florelis · 2023-11-07T19:24:06Z

src/AppInstallerCLITests/TestHooks.h

@@ -39,6 +39,9 @@ namespace AppInstaller
    namespace Repository::Microsoft
    {
        void TestHook_SetPinningIndex_Override(std::optional<std::filesystem::path>&& indexPath);
+
+        using GetARPKeyFunc = Registry::Key(*)(void*, Manifest::ScopeEnum, Utility::Architecture);


Nit: Could we use std::function here?

florelis · 2023-11-07T19:28:51Z

src/AppInstallerCLITests/PredefinedInstalledSource.cpp

+    auto result1 = source1->Search({});
+    REQUIRE(!result1.Matches.empty());
+
+    for (const auto& match : result1.Matches)


Nit: Could you add a comment about what this is loop is doing?

florelis · 2023-11-07T19:29:28Z

src/AppInstallerCLITests/PredefinedInstalledSource.cpp

+    auto result1 = source1->Search({});
+    REQUIRE(!result1.Matches.empty());
+
+    for (const auto& match : result1.Matches)


Could/should we check that there are no packages in result2 that aren't in result1?

florelis · 2023-11-07T19:31:31Z

src/AppInstallerCLITests/ArpHelper.cpp

+    Architecture architectureCallback = Architecture::Unknown;
+
+    ScopeEnum scopeTarget = ScopeEnum::User;
+    Architecture architectureTarget = Architecture::X64;


Will this test run on x86?

florelis · 2023-11-07T19:56:12Z

azure-pipelines.yml

+  # - task: VSTest@2
+  #   displayName: 'Run tests: Microsoft.Management.Configuration.UnitTests (InProc)'
+  #   inputs:
+  #     testRunTitle: Microsoft.Management.Configuration.UnitTests (InProc)
+  #     testSelector: 'testAssemblies'
+  #     testAssemblyVer2: '**\Microsoft.Management.Configuration.UnitTests.dll'
+  #     searchFolder: '$(buildOutDir)\Microsoft.Management.Configuration.UnitTests'
+  #     codeCoverageEnabled: true
+  #     platform: '$(buildPlatform)'
+  #     configuration: '$(BuildConfiguration)'
+  #     diagnosticsEnabled: true
+  #   condition: succeededOrFailed()


Use enabled: false instead of commenting it out

Suggested change

# - task: VSTest@2

# displayName: 'Run tests: Microsoft.Management.Configuration.UnitTests (InProc)'

# inputs:

# testRunTitle: Microsoft.Management.Configuration.UnitTests (InProc)

# testSelector: 'testAssemblies'

# testAssemblyVer2: '**\Microsoft.Management.Configuration.UnitTests.dll'

# searchFolder: '$(buildOutDir)\Microsoft.Management.Configuration.UnitTests'

# codeCoverageEnabled: true

# platform: '$(buildPlatform)'

# configuration: '$(BuildConfiguration)'

# diagnosticsEnabled: true

# condition: succeededOrFailed()

- task: VSTest@2

displayName: 'Run tests: Microsoft.Management.Configuration.UnitTests (InProc)'

inputs:

testRunTitle: Microsoft.Management.Configuration.UnitTests (InProc)

testSelector: 'testAssemblies'

testAssemblyVer2: '**\Microsoft.Management.Configuration.UnitTests.dll'

searchFolder: '$(buildOutDir)\Microsoft.Management.Configuration.UnitTests'

codeCoverageEnabled: true

platform: '$(buildPlatform)'

configuration: '$(BuildConfiguration)'

diagnosticsEnabled: true

enabled: false

condition: succeededOrFailed()

yao-msft · 2023-11-07T21:53:30Z

src/AppInstallerSharedLib/Public/winget/SQLiteStorageBase.h

@@ -25,6 +25,9 @@ namespace AppInstaller::SQLite
        // Gets the last write time for the database.
        std::chrono::system_clock::time_point GetLastWriteTime();

+        // Gets the identifier written to the database when it was created.
+        std::string GetDatabaseIdentifier();


Is this method used only for testing purpose?

I added it exclusively for test purposes at this time. But it seemed like an interesting enough idea to just leave it in for all databases.

Girofox · 2023-11-08T11:17:51Z

Possible trigger for debugging

manually starting backup in Windows Backup app, this instantly causes WindowsPackageManagerServer.exe running at around 25 % cpu usage for some time.
process is still running even after backup is finished, but there is a message that apps backup may be incomplete.

The log file in %LocalAppData%\Packages\Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\LocalState\DiagOutputDir grows very quickly to more than 1.5 Mb in my case too.

Cherry-pick of #3808 , then fixing up the changes (mostly namespace changes, but also bring one extra function over) for the fact that the SQLite base code moved around.

JohnMcPMS added 2 commits October 19, 2023 15:47

Most basic caching, initial tests show ~50% time improvement

b196b21

Minor changes and test validation

f2f7dd1

JohnMcPMS requested a review from a team as a code owner October 24, 2023 18:11

JohnMcPMS added 2 commits October 24, 2023 14:07

Expand test to work on server

2957a5e

Force a cache update after we install something to override the 1 sec…

50122ca

…ond wait

This comment has been minimized.

Sign in to view

JohnMcPMS added 2 commits November 1, 2023 20:44

Spelling

b91aad1

Expand tests, fix threading issues

ac1c153

This comment has been minimized.

Sign in to view

Spelling

020399b

JohnMcPMS added 8 commits November 2, 2023 20:10

Change to event driven update determination and add more tests

af2b066

Make test use different root and maybe fix pipeline pdb publishing

409d7c8

Add regex caching

1c1e50a

Merge from master

5d630f8

Make single regex static and improve version lookup efficiency

331645d

Optimize version lookups

7b6fea8

Let internal property lookups handle missing manifest

9789305

Make version comparison case insensitive to match with index

d2bb559

JohnMcPMS changed the title ~~Implement a simple cache for installed package data~~ Performance improvements Nov 4, 2023

JohnMcPMS added 4 commits November 4, 2023 23:15

Bring back a forced cache update after an install to reduce timing im…

aef1eb4

…pacts of events

Skip test to see if it is localized

506f8b3

Try disabling optimization for better crash debugging

eb2834d

Restore optimization and disable test for now

1863a5f

florelis previously approved these changes Nov 7, 2023

View reviewed changes

florelis reviewed Nov 7, 2023

View reviewed changes

PR feedback

d26ef84

JohnMcPMS dismissed florelis’s stale review via d26ef84 November 7, 2023 21:55

This comment has been minimized.

Sign in to view

florelis previously approved these changes Nov 7, 2023

View reviewed changes

yao-msft previously approved these changes Nov 7, 2023

View reviewed changes

Spelling and pipeline

20c8e88

JohnMcPMS dismissed stale reviews from yao-msft and florelis via 20c8e88 November 7, 2023 23:31

yao-msft approved these changes Nov 8, 2023

View reviewed changes

JohnMcPMS merged commit 296a53d into microsoft:master Nov 8, 2023
8 checks passed

JohnMcPMS deleted the cache-installed branch November 8, 2023 03:05

MagicAndre1981 mentioned this pull request Nov 8, 2023

Windows Package Manager Server runs on high CPU #3667

Closed

JohnMcPMS mentioned this pull request Nov 8, 2023

Performance improvements #3867

Merged

JohnMcPMS added a commit that referenced this pull request Nov 9, 2023

Performance improvements (#3867)

cddb7f8

Cherry-pick of #3808 , then fixing up the changes (mostly namespace changes, but also bring one extra function over) for the fact that the SQLite base code moved around.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements #3808

Performance improvements #3808

JohnMcPMS commented Oct 24, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

denelon commented Nov 2, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023 •

edited

Loading

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023

florelis Nov 7, 2023 •

edited

Loading

This comment has been minimized.

yao-msft Nov 7, 2023

JohnMcPMS Nov 7, 2023

Girofox commented Nov 8, 2023 •

edited

Loading

		// Creating the source reference for this is sufficient to cause the cache to be updated on next Open.
		InstalledForceCacheUpdate,

Performance improvements #3808

Performance improvements #3808

Conversation

JohnMcPMS commented Oct 24, 2023 • edited Loading

Change

Installed package caching

ICU regex caching

Version lookup (string)

Version lookup (available version)

Let property lookups handle an unknown manifest id

Validation

Microsoft Reviewers: Open in CodeFlow

This comment has been minimized.

This comment has been minimized.

denelon commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florelis Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florelis Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Girofox commented Nov 8, 2023 • edited Loading

Possible trigger for debugging

JohnMcPMS commented Oct 24, 2023 •

edited

Loading

florelis Nov 7, 2023 •

edited

Loading

florelis Nov 7, 2023 •

edited

Loading

Girofox commented Nov 8, 2023 •

edited

Loading