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

DYN-4231 Fast Path for sweep during FullGC #11923

Merged
merged 15 commits into from
Nov 8, 2021

Conversation

saintentropy
Copy link
Contributor

@saintentropy saintentropy commented Aug 9, 2021

Purpose

https://jira.autodesk.com/browse/DYN-4231

The purpose of this PR is to optimize the Sweep pass for the FullGC pass that occurs after any change to the graph that causes re-execution. Specifically this optimization does the rolling:

  • introduces a specific Call method to the Executive (CallDispose) tailored specifically for dispatching Dispose calls vs allowing these calls from the GC to pass through the generic Callr. Additionally it introduces a specific Dispatch method to the Callsite ('DispatchDispose) also tailored to Dispatch Dispose calls vs Dispatch. In both case the normal overhead of the CallrandDispatchis much higher and .NET memory/GC intensive than the actualDispose` calls.
  • Adds caching based on object type where possible as the GC is often collecting similar types repeatedly

In testing a graph that had ~150000 items to collect in the sweep pass the time was reduced from 2s to .5s and the .NET memory allocation from 800mb to 16mb.

Todo is to validate with testing include DS object dispose.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

TBD

FYIs

@jasonstratton @QilongTang

@saintentropy saintentropy removed the WIP label Oct 8, 2021
@saintentropy saintentropy changed the title WIP: Fast Path for sweep during FullGC Fast Path for sweep during FullGC Oct 8, 2021
@saintentropy saintentropy changed the title Fast Path for sweep during FullGC DYN-4231 Fast Path for sweep during FullGC Oct 18, 2021
Comment on lines 650 to 653
exe.rmem.Push(StackValue.BuildArrayDimension(0));
exe.rmem.Push(StackValue.BuildPointer(svPtr.Pointer, svPtr.metaData));
exe.rmem.Push(StackValue.BuildInt(1));

Copy link
Contributor

Choose a reason for hiding this comment

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

Why have these lines been removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not necessary with the new route and would be a memory leak. Also FYI in Dynamo today it does seem to have a memory leak regardless. We always add 3 items but only ever Pop two.

Copy link
Contributor

@sm6srw sm6srw left a comment

Choose a reason for hiding this comment

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

Many thanks @aparajit-pratap for your insights. I have been trying to break this in various ways, unsuccessfully so far. It works well. In many cases where it currently takes 10-15 times longer to dispose objects than it takes to create them it now takes about the same time to create and dispose. I am also seeing about 20% less memory being used.

@sm6srw
Copy link
Contributor

sm6srw commented Nov 5, 2021

@saintentropy You have 4 test failures now.

disposeArguments[0] = stackValue;

//EXECUTE
return finalFep.Execute(null, disposeArguments, null, runtimeCore);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the context (first arg) passed as null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only because for this execution path it was not referenced.

@saintentropy saintentropy merged commit 69cb607 into DynamoDS:master Nov 8, 2021
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