Reduce the number of calls to UpdateTimestampsCache #1467

gliljas · 2017-11-30T22:50:35Z

This needs a bit of review, since it maybe just goes from wrong to less wrong. The problem is that 2nd level UpdateTimestampCache invalidation is performed twice per action. Insert 200 items, the cache is invalidated 400 times. Unnecessary for "cheap" caches, a significant performance hit for caches with latency.

This PR reduces the number from 400 to 2. It may, due to its design, perform it even on things that fail to execute, but that only has unnecessary invalidation as a side effect.

I see now that a bit of whitespace has been added (probably by R#). The wrong whitespaces?

hazzik

Just comments on the code style. I did not review the actual changes in PR.

hazzik · 2017-11-30T22:52:28Z

src/NHibernate.Test/DebugSessionFactory.cs

+		public UpdateTimestampsCache UpdateTimestampsCache
+		{
+			get => _updateTimestampsCache ?? ActualFactory.UpdateTimestampsCache;
+			set { _updateTimestampsCache = value; }


Please use consistent code style. Either both need to be in brackets or to be lambdas.

hazzik · 2017-11-30T22:57:09Z

src/NHibernate/Async/Engine/ActionQueue.cs

@@ -24,15 +24,6 @@ namespace NHibernate.Engine
 	using System.Threading;
 	public partial class ActionQueue
 	{
-
-		public Task AddActionAsync(BulkOperationCleanupAction cleanupAction, CancellationToken cancellationToken)


Please manually add back this method (into non-async part) as an async-to-sync wrapper to AddAction. And make it obsolete.

It happened automatically, but I'll try what you said.

Unfortunately, making it obsolete didn't stop the async generator from generating calls to it, and since that is treated as an error, it prevents compilation. I fixed it manually, for the sake of the review, but is there a way to prevent this?

fredericDelaporte · 2017-12-01T00:07:03Z

The whitespace changes I have seen match editorconfig, they are "good" ones. But for easing reviews, global reformatting of files should be avoided. These changes normally do not occurs without an explicit call to a global reformatting action.

(That annoys me to not be able to fire a global reformat when I want to reformat the code I have just added, but well, global reformat annoys reviewers instead.)

hazzik · 2017-12-01T01:54:16Z

src/NHibernate/Engine/ActionQueue.cs

@@ -169,6 +187,10 @@ public void ExecuteInserts()
 		/// </summary>
 		public void ExecuteActions()
 		{
+			if (session.Factory.Settings.IsQueryCacheEnabled)


ExecuteInserts (a few lines above) should call this as well.

It seems that it shall be called after execute

Also Execute(action) can be called directly. Which then will not "pre-invalidate" the cache.

Bummer. I had made a mental note of it, but forgot.

hazzik · 2017-12-01T02:18:56Z

src/NHibernate/Engine/ActionQueue.cs

@@ -56,6 +57,8 @@ public ActionQueue(ISessionImplementor session)

 			afterTransactionProcesses = new AfterTransactionCompletionProcessQueue(session);
 			beforeTransactionProcesses = new BeforeTransactionCompletionProcessQueue(session);
+
+			querySpaces = new HashSet<string>();


I think it is better to utilize afterTransactionProcesses.querySpacesToInvalidate instead of creating an additional hashset.

Or opposite.

Or opposite. As you have 2 collections which hold exactly the same data.

Also, you've forgot to clear the field.

hazzik · 2017-12-02T07:48:21Z

src/NHibernate/Engine/ActionQueue.cs

@@ -42,6 +43,7 @@ public partial class ActionQueue

 		private readonly AfterTransactionCompletionProcessQueue afterTransactionProcesses;
 		private readonly BeforeTransactionCompletionProcessQueue beforeTransactionProcesses;
+		private readonly HashSet<string> executedSpaces;


@gliljas I'm still not sure why you need an additional collection for executedSpaces if you can use AfterTransactionCompletionProcessQueue.querySpacesToInvalidate (just make it internal) which holds exactly the same data?

@hazzik I think that it's a bit wrong to to piggyback on a class that is not involved in the PreInvalidation. However, I have now moved them together, so that the spaces added to
AfterTransactionCompletionProcessQueue comes from the same collection.

I still don’t understand why you can not use the same collection

hazzik · 2017-12-03T21:07:13Z

src/NHibernate/Engine/ActionQueue.cs

+			if (session.Factory.Settings.IsQueryCacheEnabled)
+			{
+				var spaces = executedSpaces.ToArray();
+				afterTransactionProcesses.AddSpacesToInvalidate(spaces);


I would make the parameter of AddSpacesToInvalidate IReadOnlyCollection<string> to avoid calling .ToArray().

Or, actually, remove that method and use executedSpaces directly instead of querySpacesToInvalidate collection.

hazzik · 2017-12-03T21:42:00Z

@gliljas I'll make a PR into your branch. It is apparently easier to code than explain:)

gliljas · 2017-12-04T16:44:42Z

@hazzik Console.WriteLine("Please do")

hazzik · 2017-12-05T01:17:31Z

src/AsyncGenerator.yml

@@ -119,6 +119,8 @@
    - name: GetFieldValue
    - name: IsDBNull
    - name: WriteLine
+    - name: AddAction
+      containingTypeName: ActionQueue


@maca88 we need a way to ignore a method if it's async counterpart is marked as obsolete. This would happen a lot in the future.

I will add an option to make this possible in the following days, most likely tomorrow.

Thanks @maca88. Don't need to rush.

The new option was added and released with the version 0.7.0. To ignore all obsolete async counterparts add the following lines inside the analyzation property:

ignoreAsyncCounterparts: - rule: Obsolete

where Obsolete is a method rule defined as:

- filters: - hasAttributeName: ObsoleteAttribute name: Obsolete

hazzik · 2017-12-05T01:40:22Z

@gliljas here is the PR: gliljas#1

I have removed the cache invalidation from the AfterTransactionCompletionProcessQueue (which is not really a queue, btw). I think this way it's much clear as it's not really AfterTransactionCompletionProcessQueue's job to invalidate caches.

Move invalidation of caches to ActionQueue

hazzik · 2017-12-06T22:51:46Z

There is a bug in NSubstitute, which they seem do not want to acknowledge: nsubstitute/NSubstitute#70. I'll make a necessary workaround.

Fixed assertions in unit test.

hazzik · 2017-12-06T22:58:24Z

@gliljas haha, I've made almost the same adjustments. Need to regenerate the tests thou.

gliljas · 2017-12-06T23:01:37Z

@hazzik Ah, crap. Will do.

gliljas · 2017-12-06T23:23:39Z

src/NHibernate.Test/Async/SecondLevelCacheTest/InvalidationTests.cs


 		}

+		private bool IsRight(HashSet<string> x)


Hmm...added when debugging

hazzik

LGTM.

hazzik · 2017-12-07T02:56:51Z

src/AsyncGenerator.yml

@@ -4,8 +4,6 @@
  applyChanges: true
  analyzation:
    methodConversion:
-    - conversion: Ignore
-      hasAttributeName: ObsoleteAttribute


As for now we want to generate async counterparts for obsoleted methods (as they were here before the methods were marked obsolete)

hazzik · 2017-12-07T02:57:39Z

src/AsyncGenerator.yml

@@ -96,6 +94,9 @@
    - conversion: Ignore
      name: Exists
      containingTypeName: AbstractCollectionPersister
+    - conversion: Ignore
+      name: QuoteTableAndColumns
+      containingTypeName: SchemaMetadataUpdater


This method was obsoleted in 5.0, and did not have an async counterpart, so ignore it explicitly.

hazzik · 2017-12-07T02:59:10Z

src/AsyncGenerator.yml

@@ -119,6 +120,8 @@
    - name: GetFieldValue
    - name: IsDBNull
    - name: WriteLine
+    ignoreAsyncCounterparts:
+    - rule: Obsolete


New from 0.7.0. From now on we want to generate async counterparts for obsolete members, but do not want to call them. Also, we would want to add obsolete members by hands where they ceased to be generated.

fredericDelaporte · 2017-12-07T08:34:19Z

I have relaunched the failed builds, they have got a network connectivity issue.

hazzik · 2017-12-07T10:42:10Z

Thanks @fredericDelaporte

I think the title of the PR needs to be changed to something more descriptive

fredericDelaporte · 2017-12-07T11:39:45Z

src/NHibernate.Test/TestExtensions.cs

+			else
+			{
+				//camel cased field
+				var name = method.Member.Name.Substring(0, 1).ToLowerInvariant() + method.Member.Name.Substring(1);


I am not convinced by the need of SetPropertyUsingReflection in its current form. I would rather have just a SetField(object instance, string name) helper for ceasing re-coding the reflection. (Generic if you prefer, this may allow handling static fields too by passing a null object.)
Assuming camel case for the backing field, while we have more and more readonly auto-properties, camel case underscore names (which should be the default naming but due to Java porting we often have camel case instead), is not good in my opinion. We may try to be smarter and handle many conventions (as do the property accessor strategy in mappings), but I am not convinced it is worth it. And readonly auto-properties will remain hard to handle.

At the very least if we keep this, it should throw when not finding the field.

This was a method I just copied straight from my own at-work code, where we usually use it to set properties on NH mapped domain classes, where the setters are hidden. It uses the current form to provide chaining, so that settings many properties doesn't become a burden. The lambda is just refactoring friendly (we don't use the field version, it was added here)

I don't mind having a simpler implementation.

At the very least if we keep this, it should throw when not finding the field.

Absolutely. That was an omission.

So maybe adjust it by changing

if (field != null) { field.SetValue(instance, value); }

To:

if (field == null) var field = typeof(T).GetField("_" + name, BindingFlags.NonPublic | BindingFlags.Instance); if (field == null) throw new InvalidOperationException($"Unable to find a setter or a backing field for property {method.Member.Name}"); field.SetValue(instance, value);

fredericDelaporte · 2017-12-07T12:15:50Z

src/NHibernate/Cache/UpdateTimestampsCache.cs

+		}
+
+		[MethodImpl(MethodImplOptions.Synchronized)]
+		public virtual void PreInvalidate(IReadOnlyCollection<string> spaces)


I do not think this class is meant to be extended. (It should likely be sealed, but it could be done only for 6.0.) Is virtual necessary for NSubstitute? If yes I quite dislike to have to do that for testing purpose.
Otherwise why should it be virtual?
(Same for the two next virtual methods.)

I'm more in the "virtual should be the default" camp, but yes, it's for NSubstitute. It's very hard to create real unit tests in NHibernate, since everything is so coupled. I figured making these methods virtual was a small price to pay for coming a bit closer to a unit test. An alternative (which I explored) is to create mocks of the underlying ICache, but that's actually quite hard, since the provider is instantiated using reflection.

Well, not as big as making something more visible just for test. So ok.

Well, not as big as making something more visible just for test. So ok.

@fredericDelaporte I was unsuccessfully trying to comprehend your last comment. Could you please explain?

Switching something to virtual just for tests is not, in most cases, a big concern. (At least, far from being as big as some other "just for tests" changes we have already seen in NHibernate, and which were raising the visibility of some members.)
So let it be for this case here.

Thanks for making the fixes, @hazzik

hazzik · 2017-12-07T22:04:57Z

Removed TestExtensions and made UpdateTimestampsCache.Clear/ClearAsync virtual.

hazzik reviewed Nov 30, 2017

View reviewed changes

hazzik added this to the 5.1 milestone Nov 30, 2017

hazzik reviewed Dec 1, 2017

View reviewed changes

Batching and distincting cache invalidation calls

391d231

gliljas force-pushed the unnecessarycachecalls branch from 1b5fce7 to 391d231 Compare December 2, 2017 00:20

Fixed ExecuteInserts

616a1dd

hazzik reviewed Dec 2, 2017

View reviewed changes

hazzik added c: Core p: Major t: Improvement labels Dec 3, 2017

Consolidated adding of queryspaces to be invalidated

38e569c

hazzik reviewed Dec 3, 2017

View reviewed changes

gliljas closed this Dec 4, 2017

gliljas reopened this Dec 4, 2017

hazzik reviewed Dec 5, 2017

View reviewed changes

Move invalidation of caches to ActionQueue

bcf4965

Merge pull request #1 from hazzik/nhibernategh-1467

5878986

Move invalidation of caches to ActionQueue

Stronger typing of invalidation methods.

7b04696

Fixed assertions in unit test.

gliljas mentioned this pull request Dec 6, 2017

Upgrade AsyncGenerator to 0.7.0 #1474

Merged

Async unit test and invalidation

b0c5aa2

gliljas commented Dec 6, 2017

View reviewed changes

Debug method removed

7e3b9fc

hazzik added 2 commits December 7, 2017 15:25

Merge branch 'master' into nhibernategh-1467

3defece

Mark methods of UpdateTimestapsCache obsolete

4f24faa

hazzik previously approved these changes Dec 7, 2017

View reviewed changes

hazzik requested a review from fredericDelaporte December 7, 2017 03:00

Fix async tests

9607294

hazzik dismissed their stale review via 9607294 December 7, 2017 03:16

Adjust tests to ensure generated async tests pass

63dc8cd

hazzik previously approved these changes Dec 7, 2017

View reviewed changes

fredericDelaporte reviewed Dec 7, 2017

View reviewed changes

hazzik added 2 commits December 8, 2017 11:03

Make Clear/ClearAsync virtual

181bba1

Remove TestExtensions

298bb22

hazzik dismissed their stale review via 298bb22 December 7, 2017 22:04

Merge branch 'master' into nhibernategh-1467

ed63678

gliljas changed the title ~~Altered handling of cache invalidation~~ Reduce the number of calls to UpdateTimestampsCache Dec 7, 2017

Fix getting updateTimestampsCache field

93ae332

fredericDelaporte approved these changes Dec 7, 2017

View reviewed changes

Merge branch 'master' into unnecessarycachecalls

fd5c66e

hazzik merged commit 73797df into nhibernate:master Dec 8, 2017

hazzik added the r: Fixed label Dec 8, 2017

gliljas deleted the unnecessarycachecalls branch December 8, 2017 08:40

gliljas mentioned this pull request Jan 2, 2019

Query space invalidation doesn't work for bulk actions #1953

Closed

Reduce the number of calls to UpdateTimestampsCache #1467

Reduce the number of calls to UpdateTimestampsCache #1467

Uh oh!

Conversation

gliljas commented Nov 30, 2017

Uh oh!

hazzik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Dec 3, 2017

Uh oh!

gliljas commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Dec 5, 2017

Uh oh!

hazzik commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gliljas commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fredericDelaporte commented Dec 1, 2017 •

edited

Loading

hazzik Dec 1, 2017 •

edited

Loading

hazzik Dec 5, 2017 •

edited

Loading

hazzik commented Dec 6, 2017 •

edited

Loading

hazzik commented Dec 6, 2017 •

edited

Loading

fredericDelaporte Dec 7, 2017 •

edited

Loading