Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Add support for IAsyncEnumerable #2274

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

maca88
Copy link
Contributor

@maca88 maca88 commented Nov 16, 2019

Limitations: The added Linq extension method AsAsyncEnumerable do not support PostExecuteTransformer as the transformer operates with IEnumerable<>.

Fixes: #2267

Added WIP as the async generator has some issues generating obsolete overloads and tests with IAsyncEnumerable.

src/NHibernate/IQuery.cs Outdated Show resolved Hide resolved

using (SuspendAutoFlush()) //stops flush being called multiple times if this method is recursively called
{
return plan.PerformAsyncIterate<T>(queryParameters, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this will behave....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you talking about? Is it about using (SuspendAutoFlush())? Yeah this code seems suspicious - if this suspend is really required ( as this code looks broken to me even for sync case - it's surely not scoped for items iterations) it needs to be done inside iterator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm talking about that.

Copy link
Contributor Author

@maca88 maca88 Mar 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As now PerformAsyncIterate and PerformIterate do not execute any query, the auto flush logic was moved into the initialization of the enumerator, so that the auto flush logic will be triggered before the query is executed.
I checked how hibernate does that and they are also suspending only for the first query that is executed. I didn't found any scenario that would need to perform this logic inside the iteration.

@hazzik
Copy link
Member

hazzik commented Nov 16, 2019

Looks good so far.

@bahusoid

This comment has been minimized.

@maca88

This comment has been minimized.

@bahusoid

This comment has been minimized.

@maca88

This comment has been minimized.

@bahusoid

This comment has been minimized.

@maca88

This comment has been minimized.

src/NHibernate/Impl/AsyncEnumerableImpl.cs Outdated Show resolved Hide resolved
src/NHibernate/NHibernateUtil.cs Outdated Show resolved Hide resolved
src/NHibernate/Linq/DefaultQueryProvider.cs Show resolved Hide resolved

using (SuspendAutoFlush()) //stops flush being called multiple times if this method is recursively called
{
return plan.PerformAsyncIterate<T>(queryParameters, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you talking about? Is it about using (SuspendAutoFlush())? Yeah this code seems suspicious - if this suspend is really required ( as this code looks broken to me even for sync case - it's surely not scoped for items iterations) it needs to be done inside iterator.

}

bool sessionDefaultReadOnlyOrig = _session.DefaultReadOnly;
_session.DefaultReadOnly = _readOnly;
Copy link
Member

@bahusoid bahusoid Nov 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really make sense to do it for each processed item? Maybe it should be done once for iterations (so this logic should be moved to GetAsyncEnumerables or even to JoinedAsyncEnumerable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really make sense to do it for each processed item?

The user may change the DefaultReadOnly value while iterating, which may cause a readonly query to load entites as writable, if DefaultReadOnly is not set for each item.

@hazzik
Copy link
Member

hazzik commented Nov 17, 2019

So, with the latest iteration of changes this has become a close resemblance of "Future" feature, right?

@maca88
Copy link
Contributor Author

maca88 commented Nov 17, 2019

The last iteration of changes applied the same logic in terms of when the query is executed to the current Enumerable methods, which is similar how "Future" feature work, right.

@maca88
Copy link
Contributor Author

maca88 commented Feb 29, 2020

Rebased and re-generated with AsyncGenerator 0.18.1

@maca88
Copy link
Contributor Author

maca88 commented Mar 1, 2020

Only now I realized that PerformIterate and now PerformAsyncIterate trigger N+1 queries. For example the following query:

s.CreateQuery("from Simple").Enumerable<Simple>()

will trigger one query that will retrieve all ids and then on each MoveNext call a separate query will be triggered for the current id, if the entity is not in the cache (hibenate does the same). Hibernate has also scroll method that does what I thought Enumerable does, which executes only one query. For me, the behavior of scroll method is more suitable for Enumerable and AsyncEnumerable but it would require to port the feature from hibernate, which is a topic for a separate PR.

@maca88 maca88 changed the title WIP - Add support for IAsyncEnumerable Add support for IAsyncEnumerable Mar 1, 2020
@maca88
Copy link
Contributor Author

maca88 commented Mar 1, 2020

Rebased.

@bahusoid
Copy link
Member

Only now I realized that PerformIterate and now PerformAsyncIterate trigger N+1 queries.

That sucks :( It's not very useful for entities but I guess it's still can be useful for scalar values...

For me, the behavior of scroll method is more suitable for Enumerable and AsyncEnumerable but it would require to port the feature from hibernate, which is a topic for a separate PR.

Yeah. And I would just replace Enumerable implementation with scroll. Current enumerable entity handling is not intuitive at all.

@maca88
Copy link
Contributor Author

maca88 commented Mar 25, 2020

Yeah. And I would just replace Enumerable implementation with scroll

One issue that I didn't checked, is how scroll behave when the query has collection fetches. Hibernate uses FetchingScrollableResultsImpl, but I didn't tested it yet to understand how it works.

@fredericDelaporte
Copy link
Member

So, with the latest iteration of changes this has become a close resemblance of "Future" feature, right?

The last iteration of changes applied the same logic in terms of when the query is executed to the current Enumerable methods, which is similar how "Future" feature work, right.

The main purpose of the Future feature is not deferred execution but batching together queries. (Future also features a very loose coupling between code parts defining the queries, contrary to the recent query batcher which requires referring to the same batcher reference for adding queries into it, but as such also helps knowing exactly what is batched together.)
Deferred execution is just a side effect of how Future works.

This IAsyncEnumerable feature does nothing to batch queries together, so it is still two quite different features.

Anyway, it appears this "enumerable" feature is not very attractive with its current performances issues.

Is it the reason for this PR to be stalled? Or should its conflicts be resolved for allowing merging it?

@maca88 maca88 changed the title Add support for IAsyncEnumerable WIP - Add support for IAsyncEnumerable Oct 5, 2020
@maca88
Copy link
Contributor Author

maca88 commented Oct 5, 2020

Is it the reason for this PR to be stalled?

Yes, the scroll feature from Hibernate should be ported first, so that it can be used in this PR to avoid N+1 issue. I haven't started working on it yet as my current priority is the Include method. Changed to WIP until the scroll feature is ported.

@zlsmith86
Copy link

Is there an issue to track the porting of the scroll feature?

@Harpush
Copy link

Harpush commented Apr 17, 2024

Any progress here with the scroll feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for IAsyncEnumerable
6 participants