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

Add InterruptibleLazy #16179

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Add InterruptibleLazy #16179

merged 2 commits into from
Nov 2, 2023

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Oct 25, 2023

Continues #16137 and adds Lazy implementation that doesn't cache exceptions, allowing it to be used when OperationCancelledException may be raised inside the factory function. I've initially added it to various IL types, but ended up transitively updating other places, so no conversion between lazy types is needed. It also allows to cancel in more places in future if we decide to add more cancellation points via Cancellable or other way.

It also adds tests for cancellation inside metadata reading in various places during project analysis startup and during code analysis. These tests show that previously cancelled requests now don't have any impact on subsequent requests and these requests return correct info. These tests have also shown some performance issues in metadata reading (see #16166), which is good, since we can improve these parts of the compiler too and even assert it with these tests.

Next steps may be these:

  • Check LazyWithContext. It seems it's redundant to create it just for keeping the exception ranges, as errorRecovery also attaches relatively good range to errors. It needs to be checked and evaluated, though.

@vzarytovskii
Copy link
Member

Out of curiosity, have you compared it to previous version in terms of perf/allocations? Is it any significantly different?

@auduchinok
Copy link
Member Author

auduchinok commented Oct 30, 2023

Out of curiosity, have you compared it to previous version in terms of perf/allocations? Is it any significantly different?

@vzarytovskii I haven't benchmarked it, but from the analysis of the changes there shouldn't be much difference.

The F# built-in lazy allocates one more Func instance everywhere to pass it to the BCL Lazy which keeps it, so the interruptible implementation saves some memory. Otherwise the implementation is very similar to the BCL Lazy:

  • each instance have fields for the value, factory, and a sync object
  • each instance allocates a new sync object
  • there's a non-syncronized attempt to get calculated value first
  • if not calculated, there's a lock/Monitor block that implements double checked locking

We can save one more field and allocation of the sync objects to save even more memory, if we decide that it'd be OK to lock on this inside these lazy objects, but I'm OK with discussing/doing it separately.

@abonie
Copy link
Member

abonie commented Oct 30, 2023

How does launching heavy operations in the checker and then immediately cancelling them affects GC or allocations in general?

@auduchinok
Copy link
Member Author

auduchinok commented Oct 30, 2023

How does launching heavy operations in the checker and then immediately cancelling them affects GC or allocations in general?

I think it depends on a particular place where a cancellation happens, but in a typical scenario it shouldn't add overhead compared to a normal subsequent request with a different source or project options. What I think important here is if a request is cancelled, then we're not interested in its result anymore and it's also likely that the world has changed in some way: it'll will likely need to do many things again for a fresh analysis anyway. One of the main ideas of #16137, of which this PR is a continuation, is to make it possible to prevent access to an outdated state, and this PR allows it in more cases.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

To me this looks promising. Thanks!

@T-Gro T-Gro merged commit a47975f into dotnet:main Nov 2, 2023
24 checks passed
@auduchinok auduchinok deleted the il-oce branch November 2, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants