Fix Missing take and optimize some captures #347

to11mtm · 2024-02-10T17:59:08Z

May help with #346

Changes

Fixes missing TAKE on EventsByTag Query for Tag table case.
Provides overload for ExecuteInTransactionAsync that takes input state to minimize captures
Fixed a few spots where we weren't using capture-minimizing APIs completely
Tried to do a few other capture related optimizations and remove a double-prop access on read paths.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

This change follows the Akka.NET API Compatibility Guidelines.
This change follows the Akka.NET Wire Compatibility Guidelines.
I have reviewed my own pull request.

Benchmarks

Will see what I can do...

to11mtm

Self review with some food for thought.

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

to11mtm · 2024-02-10T18:11:56Z

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

                }
            ).Via(_deserializeFlow);
        }
+
+
+        internal async Task<List<JournalRow>> ExecuteEventQuery(QueryArgs queryArgs)


This is intentionally moved into it's own method,

Among other things, Another memory optimization for us to consider is -not- using Akka streams in the Unfold internal read calls; Where this gets tricky, is that marshalling via Akka streams pipeline, neatly solves problems where a lot of DB Providers aren't really all that async 🙃.

SQLite (Fair enough,) I think some oracle providers, IDK about MySQL...

I can fold back in for now if needed, but it does make the pipeline at least a little bit neater.

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

to11mtm · 2024-02-10T18:20:47Z

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

+                                            (jr, jtr) => (jr.Ordering == jtr.OrderingId),
+                                            (jr, jtr) => jr)
+                                        .OrderBy(r => r.Ordering)
+                                        .Take(txInput.Max);


So, We didn't have a take here.

However, AFAIR, the LINQ query syntax tends to be more finicky with captures/closures than (carefully) plotted out L2Db Method syntax. However, if the lambda syntax somehow is producing something needed for a specific DB, we can go back to it. NVM It's not playing nice and we can deal with that later.

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

src/Akka.Persistence.Sql/Query/Dao/QueryArgs.cs

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

to11mtm · 2024-02-10T19:00:10Z

src/Akka.Persistence.Sql/Db/AkkaPersistenceDataConnectionFactory.cs

            _cloneConnection = new Lazy<AkkaDataConnection>(
                () => new AkkaDataConnection(
-                    _opts.ConnectionOptions.ProviderName,
-                    new DataConnection(_opts)));
+                    opts.ConnectionOptions.ProviderName,
+                    new DataConnection(opts)));


subtle thing; now we are capturing just the DataOptions reference rather than the factory itself...

IDK how much the GC really cares but it sounds like the sort of thing that can annoy a cycle detector,

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

to11mtm · 2024-02-10T19:16:54Z

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

-                                        from lp in tagTable.Where(jtr => jtr.OrderingId == r.Ordering).DefaultIfEmpty()
-                                        where lp.OrderingId > input.offset &&
-                                              lp.OrderingId <= input.maxOffset &&
+                                        from r in connection.GetTable<JournalRow>()


It would still be nice to convert this to method syntax later, if only because it's something like 300 lines of IL as it stands 😅

to11mtm · 2024-02-10T19:56:28Z

src/Akka.Persistence.Sql/Extensions/ConnectionFactoryExtensions.cs

+        internal static Task<T> ExecuteWithTransactionAsync<TState,T>(
+            this DbStateHolder factory,
+            TState state,
+            Func<AkkaDataConnection, CancellationToken, TState, Task<T>> handler)
+        {
+            return factory.ConnectionFactory.ExecuteWithTransactionAsync(state, factory.IsolationLevel, factory.ShutdownToken, handler);
+        }


Convenience method; since we are using the stateholder to avoid other captures, may as well add some sugar to go with it and make the main DAO cleaner.

to11mtm · 2024-02-10T19:57:14Z

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

+
+        internal static async Task<List<JournalRow>> ExecuteEventQuery(DbStateHolder stateHolder, TagMode tagMode, QueryArgs queryArgs)
+        {
+            return tagMode != TagMode.TagTable


This is ugly and IDK how I feel but it's the easiest way to get more captures out of the way, short of some more significant refactoring where we had some different ways to compose without going back to Func<> madness...

This is ugly

Use an if statement instead of a ternary operator and it'll look better, I swear :p

The comment that was somehow left unposted, I wound up moving these back into separate methods.

Aaronontheweb

Generally LGTM, but did leave some comments

Aaronontheweb · 2024-02-13T14:09:04Z

src/Akka.Persistence.Sql/Db/AkkaPersistenceDataConnectionFactory.cs

            _cloneConnection = new Lazy<AkkaDataConnection>(
                () => new AkkaDataConnection(
-                    _opts.ConnectionOptions.ProviderName,
-                    new DataConnection(_opts)));
+                    opts.ConnectionOptions.ProviderName,
+                    new DataConnection(opts)));


Aaronontheweb · 2024-02-13T14:10:19Z

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

-                        async (connection, token) =>
+                    return await input._dbStateHolder.ExecuteWithTransactionAsync(
+                        input.maxTake,
+                        async (connection, token,take) =>


Can we just return a Task here instead of async / await ?

I'm honestly not sure, concerns aside from potential stack trace confusion on errors would be whether the fact this func is actually invoked inside two using contexts... Better safe than sorry when it comes to that stuff IMO.

Some discussions on this are here:

https://stackoverflow.com/questions/24110213/difference-between-returning-and-awaiting-a-task-in-an-async-method

https://code-maze.com/charp-difference-between-returning-and-awaiting-a-task/

Would the performance boost for each query execution be worth it, does it outweight the possible exception stack trace confusion?

From my experience, the "proper" stack trace from an exception thrown from within an awaited Task is confusing in itself, since it is thrown from inside the async...await FSM and most often it will be a detached from the actual code that invoked it (when the thrown exception is caught by the debugger), so I can't really say that it is an actual improvement in debugging experience. But then again, I could be doing it wrong.

On the other hand, the article does point out that using proper async puts the exception where it belongs, in the future when it is supposed to happen, if it indeed needs to be thrown.

I tend to agree with @to11mtm on this one, returning a Task from inside a using block is quite dangerous because we don't know just how robust the IDispose is being implemented by the 3rd party API developers.

Aaronontheweb · 2024-02-13T14:11:41Z

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs

+
+        internal static async Task<List<JournalRow>> ExecuteEventQuery(DbStateHolder stateHolder, TagMode tagMode, QueryArgs queryArgs)
+        {
+            return tagMode != TagMode.TagTable


This is ugly

Use an if statement instead of a ternary operator and it'll look better, I swear :p

src/Akka.Persistence.Sql/Query/Dao/DbStateHolder.cs

to11mtm · 2024-02-14T22:22:15Z

@Aaronontheweb if you and @Arkatufus are OK with this feel free to merge, otherwise LMK what you would like to see changed. <3

Arkatufus

LGTM

to11mtm added 2 commits February 10, 2024 12:03

Fix Take and leaky captures

c57f7c7

Further minimize capture sizes

e94254b

to11mtm force-pushed the fix-take-and-captures branch from 8276437 to e94254b Compare February 10, 2024 18:02

Cleaning

27dc9bf

to11mtm commented Feb 10, 2024

View reviewed changes

to11mtm requested review from CumpsD and Arkatufus February 10, 2024 18:37

be nicer to GC

71d45fb

to11mtm commented Feb 10, 2024

View reviewed changes

src/Akka.Persistence.Sql/Query/Dao/BaseByteReadArrayJournalDao.cs Outdated Show resolved Hide resolved

to11mtm added 2 commits February 10, 2024 14:04

Fix prior refactoring

5b4be3c

Go back to query syntax for now

224f15c

to11mtm commented Feb 10, 2024

View reviewed changes

Use Stateholder to clean more things up

26acabb

to11mtm commented Feb 10, 2024

View reviewed changes

Aaronontheweb approved these changes Feb 13, 2024

View reviewed changes

Aaronontheweb mentioned this pull request Feb 13, 2024

High memory usage #346

Open

to11mtm mentioned this pull request Feb 13, 2024

CurrentEventsByTag query on Sqlite has a weird initial delay #345

Closed

Cleanup and Feedback

ee0c83b

Merge branch 'dev' into fix-take-and-captures

db7a7df

Arkatufus approved these changes Feb 15, 2024

View reviewed changes

Arkatufus enabled auto-merge (squash) February 15, 2024 16:10

Arkatufus merged commit fe49a6b into akkadotnet:dev Feb 15, 2024
3 checks passed

Arkatufus mentioned this pull request May 8, 2024

Update RELEASE_NOTES.md for 1.5.20 release #381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Missing take and optimize some captures #347

Fix Missing take and optimize some captures #347

to11mtm commented Feb 10, 2024 •

edited

Loading

to11mtm left a comment

to11mtm Feb 10, 2024

to11mtm Feb 10, 2024 •

edited

Loading

to11mtm Feb 10, 2024

Aaronontheweb Feb 13, 2024

to11mtm Feb 10, 2024

to11mtm Feb 10, 2024

to11mtm Feb 10, 2024 •

edited

Loading

Aaronontheweb Feb 13, 2024

to11mtm Feb 13, 2024

Aaronontheweb left a comment

Aaronontheweb Feb 13, 2024

Aaronontheweb Feb 13, 2024

to11mtm Feb 13, 2024

Arkatufus Feb 15, 2024 •

edited

Loading

Arkatufus Feb 15, 2024

Aaronontheweb Feb 13, 2024

to11mtm commented Feb 14, 2024

Arkatufus left a comment

Fix Missing take and optimize some captures #347

Fix Missing take and optimize some captures #347

Conversation

to11mtm commented Feb 10, 2024 • edited Loading

Changes

Checklist

Benchmarks

to11mtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to11mtm Feb 10, 2024 • 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

to11mtm Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb left a 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

Arkatufus Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to11mtm commented Feb 14, 2024

Arkatufus left a comment

Choose a reason for hiding this comment

to11mtm commented Feb 10, 2024 •

edited

Loading

to11mtm Feb 10, 2024 •

edited

Loading

to11mtm Feb 10, 2024 •

edited

Loading

Arkatufus Feb 15, 2024 •

edited

Loading