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

Akka.Streams: memory-optimize ActorMaterializer HOCON injection #6440

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Feb 24, 2023

Changes

creating lots of materializer instances (i.e. in actors that own their own streams) results in a tremendous amount of unreclaimable HOCON-related memory - on the order of 11Gb in https://github.com/Aaronontheweb/AkkaSqlQueryCrushTest

This tries to avoid injecting the top level Akka.Streams fallback every time a materializer is used. In addition to that, we try to cache the default materializer settings parsed from the ActorSystems HOCON if none are provided.

Checklist

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

creating lots of materializer instances (i.e. in actors that own their own streams) results in a tremendous amount of unreclaimable HOCON-related memory - on the order of 11Gb in https://github.com/Aaronontheweb/AkkaSqlQueryCrushTest

This tries to avoid injecting the top level Akka.Streams fallback every time a materializer is used. In addition to that, we try to cache the default materializer settings parsed from the `ActorSystem`s HOCON if none are provided.
Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

My inline comments are self-explanatory

@Aaronontheweb
Copy link
Member Author

This should probably be back-ported to v1.4, C#11 syntax not withstanding.

@Aaronontheweb
Copy link
Member Author

For reference

image

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM, do we have any before and after memory profile?

@Aaronontheweb
Copy link
Member Author

Aaronontheweb commented Feb 24, 2023

LGTM, do we have any before and after memory profile?

I'll need to rebuild this and integrate it with #6436 to do that, and remove a materializer optimization I also made inside my app.

@Arkatufus
Copy link
Contributor

Ok, the changes are benign so we can merge them anyway

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) February 24, 2023 17:32
@Aaronontheweb
Copy link
Member Author

According to test suite, this for sure broke something

@Aaronontheweb
Copy link
Member Author

Ah, the static is the issue - doesn't work when there's multiple ActorSystems per process.

Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Reviewed changes

/// <summary>
/// Injecting the top-level Materializer HOCON configuration over and over again is expensive, so we want to avoid
/// doing it each time a materializer is instantiated. This flag will be set to true once the configuration has been
/// injected the first time.
/// </summary>
private static volatile bool _injectedConfig = false;
private static readonly ConcurrentDictionary<ActorSystem, bool> InjectedConfig = new();
Copy link
Member Author

Choose a reason for hiding this comment

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

This would be unnecessary if the materializer were an ActorSystemExtension, but since it's not - have to do it this way.

{
// Inject the top-level fallback config for the Materializer once, and only once.
// This is a performance optimization to avoid having to do this on every materialization.
system.Settings.InjectTopLevelFallback(DefaultConfig());
_injectedConfig = true;

static async Task CleanUp(ActorSystem sys)
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a static delegate to cleanup ActorSystem references after shut down

@Aaronontheweb
Copy link
Member Author

@Arkatufus re-ran my crush test with these changes - the materializer no longer even appears in the memory footprint now.

@Aaronontheweb Aaronontheweb merged commit da1fceb into akkadotnet:dev Feb 24, 2023
@Aaronontheweb Aaronontheweb deleted the materializer-hocon-injection branch February 24, 2023 18:36
@ismaelhamed
Copy link
Member

@Aaronontheweb , in the JVM they have this default system wide materializer which is the recommended, since you typically only need one materializer per application.

@Aaronontheweb
Copy link
Member Author

@Aaronontheweb , in the JVM they have this default system wide materializer which is the recommended, since you typically only need one materializer per application.

Yeah, I think we should go ahead and just do that too.

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.

3 participants