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

Fixed Lazy loader performance issues #35835

Merged
merged 8 commits into from
Mar 25, 2025

Conversation

henriquewr
Copy link
Contributor

Changed AsyncLocal to ThreadId for better performance
Fixes #35832

@henriquewr henriquewr requested a review from a team as a code owner March 24, 2025 05:11
@henriquewr henriquewr changed the title Lazy loader perf Fixed Lazy loader performance issues #35832 Mar 24, 2025
@henriquewr henriquewr changed the title Fixed Lazy loader performance issues #35832 Fixed Lazy loader performance issues Mar 24, 2025
@roji
Copy link
Member

roji commented Mar 24, 2025

@henriquewr I'm not very familiar with that particular part of the code, but async code is executed across different threads - every time the code awaits, it may continue executing on a different thread. So checking the thread ID is probably wrong here - the AsyncLocal is being used to propagate the state across different threads.

But I may be misunderstanding things here. @AndriySvyryd assigning this (and #35832) to you as you know the area best.

@AndriySvyryd
Copy link
Member

I agree with @roji, for recursive lazy loading async calls the thread might change between the outer call and the nested call leading to a deadlock. If the issue is indeed caused by AsyncLocal allocations, consider using a static instance for the case when there's only one entry in _isLoading.

and use static AsyncLocal
@henriquewr
Copy link
Contributor Author

@AndriySvyryd
I removed the threadId and now im using the static AsyncLocal
I didn't realize until now that I don't need to create a new AsyncLocal instance in order to get a unique value on this context

@AndriySvyryd
Copy link
Member

With this approach there still might be a deadlock issue with different lazy loaded navs trying to load the same nav in a nested call, but this use case is probably so rare that we don't need to worry about it.

@AndriySvyryd AndriySvyryd enabled auto-merge (squash) March 25, 2025 20:46
@AndriySvyryd AndriySvyryd disabled auto-merge March 25, 2025 21:39
@AndriySvyryd AndriySvyryd merged commit 63e55f1 into dotnet:main Mar 25, 2025
5 of 7 checks passed
@AndriySvyryd
Copy link
Member

Thanks for your contribution!

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

Successfully merging this pull request may close these issues.

Lazy loader performance issues
3 participants