Sqlite placeholders by kewillford · Pull Request #1140 · microsoft/VFSForGit

kewillford · 2019-05-09T22:28:33Z

I think this is far enough along to start taking a look at. There is a lot of new code to handle the sqlite database but the other changes should be minimal.

I would like to get some feedback on how I am setting up the class for the connection and the class for the Placeholders table. Something to keep in mind is if we are going to add ModifiedPaths to this same database, does this make it easy or is there a better way?

Currently it is structured with a GVFSDatabase class that initializes the database and keeps a pool of connections for consumers to use. The connection is wrapped in a class that implements IDbConnection so that the Dispose method can return the underlying connection to the pool. Like the following:

using (IDbConnection pooled = this.database.GetPooledConnection())
using (SqliteCommand command = pooled.CreateCommand())
{

Another question is also how we show handle exceptions. The FileBasedCollection would have try catches and throw a custom FileBasedCollectionException. Should we be doing something similar for SQLite or just let exceptions bubble up?

Changed the Count property to be a method because it is now running a SQLite query and will take longer than returning a value and it being a method makes that more obvious.

Removed some constructors that caused me some confusion when I was adding the IPlaceholderDatabase as a parameter but I'm open to adding them back in if they make sense.

The most critical part to review would be the changes in GitIndexProjection.

TODO:

kewillford · 2019-05-09T22:41:13Z

/azp run PR - Windows - Functional Tests

azure-pipelines · 2019-05-09T22:41:19Z

Pull request contains merge conflicts.

kewillford · 2019-05-09T22:41:24Z

/azp run PR - Windows - Extra

azure-pipelines · 2019-05-09T22:41:29Z

Pull request contains merge conflicts.

kewillford · 2019-05-15T18:03:17Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-05-15T18:03:22Z

No pipelines are associated with this pull request.

kewillford · 2019-05-15T19:08:26Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-05-15T19:08:32Z

No pipelines are associated with this pull request.

GVFS/GVFS.Common/Database/GVFSDatabase.cs

GVFS/GVFS.Common/PlaceholderListDatabase.cs

GVFS/GVFS.Common/Database/GVFSDatabase.cs

wilbaker · 2019-05-17T22:40:29Z

Another question is also how we show handle exceptions. The FileBasedCollection would have try catches and throw a custom FileBasedCollectionException. Should we be doing something similar for SQLite or just let exceptions bubble up?

FWIW we went with having a custom exception type for the BlobSizes database (BlobSizesException) to make it easier for callers to know what exception type(s) to expect. The database code was able to map all possible exceptions to a single well know type.

wilbaker

Just a first set of comments on the database code specifically. I still need to review the tests, and how the new databases integrate with the rest of the codebase.

GVFS/GVFS.Common/Database/GVFSDatabase.cs

GVFS/GVFS.Common/Database/IPlaceholderDatabase.cs

GVFS/GVFS.Common/GVFSConstants.cs

GVFS/GVFS.Common/Database/SqliteDatabase.cs

wilbaker · 2019-05-17T22:34:59Z

GVFS/GVFS.Common/Database/Placeholders.cs

+                command.Parameters.Add("@sha", SqliteType.Text).Value = placeholder.Sha;
+            }
+
+            command.ExecuteNonQuery();


Have you tried measuring the performance impact of using prepared statements, and if so, what did you find?

I did on the Contains method that I had and it did help. The issue here would be that you need to use the same connection that the prepare was ran against. This would require the class to keep connections around and do locking for using the commands that are prepared which would really add to the complexity so I will look into that after the PR is smoothed out.

I also ran some testing using a simple locking strategy with one prepared connection and command and things were slowed down because of the lock. The next experiment will be to have a pool of prepared connections and commands to remove the need for the lock.

GVFS/GVFS.Common/Database/Placeholders.cs

kewillford · 2019-05-21T15:29:43Z

GVFS/GVFS.UnitTests/Common/Database/PlaceholdersTests.cs

+namespace GVFS.UnitTests.Common.Database
+{
+    [TestFixture]
+    public class PlaceholdersTests


This class goes a little mock crazy so I'm not sure the value this brings vs the maintenance cost of all the mocking it is doing.

kewillford · 2019-05-22T01:34:33Z

/azp run

azure-pipelines · 2019-05-22T01:34:51Z

Azure Pipelines successfully started running 5 pipeline(s).

kewillford · 2019-05-24T23:47:12Z

/azp run PR - Windows - Functional Tests

azure-pipelines · 2019-05-24T23:47:21Z

Azure Pipelines successfully started running 1 pipeline(s).

derrickstolee · 2019-05-29T15:09:32Z

@kewillford What's the status here. Are you still doing investigations? I think this is good to go, and it's probably best to let it simmer in CI before a release.

Cc: @jrbriggs, @jamill in case they know the reason.

kewillford · 2019-05-29T15:54:09Z

@derrickstolee, @wilbaker was going to take another look today.

wilbaker

Posting comments so far, I still need to review:

Test files
The rest of GVFSDatabase.cs
PlaceholderTable.cs

GVFS/GVFS.Mount/InProcessMount.cs

GVFS/GVFS.UnitTests.Windows/GVFS.UnitTests.Windows.csproj

GVFS/GVFS.Virtualization/Projection/GitIndexProjection.cs

GVFS/GVFS.Common/PlaceholderListDatabase.cs

GVFS/GVFS.Common/Database/GVFSDatabase.cs

wilbaker

I still need to review the tests, but wanted to post the rest of my comments on the product changes first.

GVFS/GVFS.Common/Database/GVFSDatabase.cs

GVFS/GVFS.Common/Database/PlaceholderTable.cs

… for folder reexpansion

…pdating

wilbaker

Adding comments on the functional test changes, reviewing the unit tests now.

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/DiskLayoutUpgradeTests.cs

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/MacDiskLayoutUpgradeTests.cs

wilbaker

Just a few more comments on the Unit Tests

GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs

GVFS/GVFS.UnitTests/Common/Database/GVFSDatabaseTests.cs

GVFS/GVFS.UnitTests/Common/Database/PlaceholderTableTests.cs

…HashSet

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/DiskLayoutUpgradeTests.cs

wilbaker

Approved with a few more minor non-blocking comments (posted earlier today)

turbonaitis · 2019-07-12T07:48:09Z

I'm a bit late to the party, but do we have some performance numbers here? How was the old placeholder DB performing and by how much the SQLite solution improves it?

kewillford · 2019-07-12T15:41:12Z

@turbonaitis here are some number with ~1 million placeholders

Old

Load and get all placeholders: 41.253 s
Write and update placeholders: 21.940 s

SQLite

Load and get all placeholders: 2.053 s
Write and update placeholders: 23.940 s

turbonaitis · 2019-07-15T07:07:59Z

These look great, thanks for sharing!

…

On Fri, Jul 12, 2019 at 6:41 PM Kevin Willford ***@***.***> wrote: @turbonaitis <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_turbonaitis&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=pfEKxc4YDcMyKmMr7r-AL7DyCalNlWsXbqnuQGFOors&m=h8dQ6YKjlD3XUR1YrSdzzWWaQfm5PbjdVOWjXRVmPzM&s=r0ibFm2E4Wt4UcNlorq4OCCJfJUyzPBNfEk7PC40JLE&e=> here are some number with ~1 million placeholders Old Load and get all placeholders: 41.253 s Write and update placeholders: 21.940 s SQLite Load and get all placeholders: 2.053 s Write and update placeholders: 23.940 s — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_microsoft_VFSForGit_pull_1140-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DADKZKF4ODPP65ZVW5BF7BITP7CQ2DA5CNFSM4HL6PC22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ2DYDA-23issuecomment-2D510934028&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=pfEKxc4YDcMyKmMr7r-AL7DyCalNlWsXbqnuQGFOors&m=h8dQ6YKjlD3XUR1YrSdzzWWaQfm5PbjdVOWjXRVmPzM&s=41C2AaT12yNyz0oKWWzyx61Ft1c0_hcGgg-artyyOqA&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ADKZKF6ZFZAMQGXYVP6P3GLP7CQ2DANCNFSM4HL6PC2Q&d=DwMCaQ&c=r2dcLCtU9q6n0vrtnDw9vg&r=pfEKxc4YDcMyKmMr7r-AL7DyCalNlWsXbqnuQGFOors&m=h8dQ6YKjlD3XUR1YrSdzzWWaQfm5PbjdVOWjXRVmPzM&s=UwZjH-K2fw8yZDGtTPVmIt0L-QA-h6bu-FNXXUgBKnw&e=> .

kewillford added the WIP label May 9, 2019

kewillford self-assigned this May 9, 2019

kewillford force-pushed the sqlite_placeholders branch 2 times, most recently from becd8b0 to 0f9e40c Compare May 13, 2019 16:47

kewillford added this to the M153 milestone May 15, 2019

kewillford changed the title ~~[WIP] Sqlite placeholders~~ Sqlite placeholders May 15, 2019

kewillford marked this pull request as ready for review May 15, 2019 19:13

kewillford added affects: performance pr: refactoring Keeping the codebase healthy and removed WIP labels May 15, 2019

kewillford requested review from jamill, jrbriggs and wilbaker May 15, 2019 19:14

jamill reviewed May 17, 2019

View reviewed changes

GVFS/GVFS.Common/Database/GVFSDatabase.cs Outdated Show resolved Hide resolved

jamill reviewed May 17, 2019

View reviewed changes

GVFS/GVFS.Common/Database/GVFSDatabase.cs Outdated Show resolved Hide resolved

jamill reviewed May 17, 2019

View reviewed changes

GVFS/GVFS.Common/PlaceholderListDatabase.cs Outdated Show resolved Hide resolved

kewillford commented May 17, 2019

View reviewed changes

GVFS/GVFS.Common/Database/GVFSDatabase.cs Outdated Show resolved Hide resolved

wilbaker reviewed May 17, 2019

View reviewed changes

kewillford commented May 21, 2019

View reviewed changes

Add validation and tests for the sha value

051f177

wilbaker suggested changes May 29, 2019

View reviewed changes

Kevin Willford added 2 commits May 29, 2019 15:15

PR Feedback to cleanup disposing and returning connections to the pool

524da2d

Rename PlaceholderListDatabase to LegacyPlaceholderListDatabase

c6047cd

wilbaker reviewed May 29, 2019

View reviewed changes

GVFS/GVFS.Common/Database/GVFSDatabase.cs Show resolved Hide resolved

GVFS/GVFS.Common/Database/GVFSDatabase.cs Outdated Show resolved Hide resolved

GVFS/GVFS.Common/Database/PlaceholderTable.cs Show resolved Hide resolved

GVFS/GVFS.Common/Database/PlaceholderTable.cs Show resolved Hide resolved

Kevin Willford added 3 commits May 29, 2019 16:10

Revert change that VS made to remove the recursive entry

e8d3ef0

Add comments why and how we are using the HashSet of all placeholders…

ae40127

… for folder reexpansion

Make sure placeholders are removed for all cases other then Ok when u…

88056e5

…pdating

wilbaker reviewed May 29, 2019

View reviewed changes

GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs Show resolved Hide resolved

GVFS/GVFS.UnitTests/Common/Database/GVFSDatabaseTests.cs Show resolved Hide resolved

GVFS/GVFS.UnitTests/Common/Database/PlaceholderTableTests.cs Show resolved Hide resolved

Kevin Willford added 5 commits May 29, 2019 17:00

PR Feedback - put get and set on separate lines

d7f0c83

Use case insensitive database field for the path and folders to keep …

5b60148

…HashSet

PR Feedback - use GVFSHelpers.PlaceholderFieldDelimiter

19b84a0

Use platform path separators instead of hard coded

b0f3015

Add CategoryConstants.ExceptionExpected to test that was missing it

bc321ce

wilbaker reviewed May 30, 2019

View reviewed changes

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/DiskLayoutUpgradeTests.cs Outdated Show resolved Hide resolved

wilbaker reviewed May 30, 2019

View reviewed changes

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/DiskLayoutUpgradeTests.cs Outdated Show resolved Hide resolved

wilbaker approved these changes May 30, 2019

View reviewed changes

PR Feedback - clean up tests

eba8fa1

kewillford merged commit 8c61c1a into microsoft:master May 30, 2019

This was referenced Jun 4, 2019

[Linux] Add GVFS platform and build configuration for Linux #1125

Merged

Perform case-sensitive path comparisons on Linux #1232

Merged

jrbriggs mentioned this pull request Jul 12, 2019

Placeholder update during git checkout takes a long time #828

Closed

Conversation

kewillford commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kewillford commented May 9, 2019

Uh oh!

azure-pipelines bot commented May 9, 2019

Uh oh!

kewillford commented May 9, 2019

Uh oh!

azure-pipelines bot commented May 9, 2019

Uh oh!

kewillford commented May 15, 2019

Uh oh!

azure-pipelines bot commented May 15, 2019

Uh oh!

kewillford commented May 15, 2019

Uh oh!

azure-pipelines bot commented May 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilbaker commented May 17, 2019

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilbaker May 17, 2019

Choose a reason for hiding this comment

Uh oh!

kewillford May 20, 2019

Choose a reason for hiding this comment

Uh oh!

kewillford May 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kewillford May 21, 2019

Choose a reason for hiding this comment

Uh oh!

kewillford commented May 22, 2019

Uh oh!

azure-pipelines bot commented May 22, 2019

Uh oh!

kewillford commented May 24, 2019

Uh oh!

azure-pipelines bot commented May 24, 2019

Uh oh!

derrickstolee commented May 29, 2019

Uh oh!

kewillford commented May 29, 2019

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

kewillford commented May 9, 2019 •

edited

Loading