Major Roslyn Improvements to Memory and Running Time #25

TravisOnGit · 2020-08-14T18:53:03Z

Roslyn Improvements Summary:

Memory consumption down from 75MB per unique expression to a flat 25MB plus 0.3MB~ per unique expression. This is a 250x improvement or 0.4% memory consumed.
Running time of first execution down from 2 seconds for each unique expression to a flat 2 seconds plus 15ms per unique expression. This is a 133x improvement or 0.75% running time.
All changes are under the covers. No changes required by applications using Forge.

Feature breakdown:

ParentScript
- The parentScript that, upon initialization, gets Created, RunAsync, and added to the ScriptCache.
- All Executed expressions are continued from the parentScript, avoiding additional compiles.
- Before improvement - each unique expression would cost 25MB, take 2 seconds on first execution, then 0ms on further executions.
- After improvement - only the parentScript costs 25MB and takes 2 seconds. Each unique expression costs < 0.5MB, takes 15ms on first execution, then 0ms on further executions.
ParentScriptTask
- The parentScriptTask kicks off initializing the Roslyn parentScript asynchronously, allowing ExpressionExecutor to construct very quickly.
- This initialization time is saved if the application takes time to initialize before the first WalkTree call.
- Before improvement - 1800ms for first initialize.
- After improvement - 2ms for first Initialize. 0ms for further initializes (new TreeWalkerSessions using same ScriptCache).
ParentScriptCode
- Code that Forge uses to prime the parentScript on initialization.
- Evaluating some code moves roughly 600ms from the first expression to asynchronously initializing the parentScript.
ScriptCache rework
- ScriptCache now caches Scripts that come from the parentScript.ContinueWith. ScriptCache still caches one Script per unique Expression, but these no longer get compiled.
- ScriptCache now caches the ParentScript as well on initialization.
MetadataReferenceResolver
- This added ScriptOption causes Roslyn to not load missing references.
- Before improvement - it costs about 75MB to initialize, then 0.5-1.5MB per unique expression.
- After improvement - it costs about 25MB to initialize, then 0.05-0.5MB per unique expression.

…ession to a flat 25MB plus 0.3MB~ per unique expression. Running time of first execution down from 2 seconds for each unique expression to a flat 2 seconds plus 15ms per unique expression.

abhayketkar

zhengmu · 2020-08-14T21:15:03Z

Forge.TreeWalker/src/TreeWalkerParameters.cs

            if (forgeState == null) throw new ArgumentNullException("forgeState");
            if (callbacks == null) throw new ArgumentNullException("callbacks");
            if (token == null) throw new ArgumentNullException("token");

            this.SessionId = sessionId;
-            this.JsonSchema = jsonSchema;
+            this.ForgeTree = forgeTree;


just to confirm, looks like you just switched line 140-152 with line 169-181. Besides these, only some comments are changed, right? #Closed

I swapped the constructors to put the ForgeTree one on top. Just because it is preferred to use that one. No functional changes.

In reply to: 470868342 [](ancestors = 470868342)

zhengmu · 2020-08-14T21:16:14Z

Forge.TreeWalker/src/ExpressionExecutor.cs

        /// </summary>
-        private List<Type> dependencies;
+        public static string ParentScriptCode = "(1+1).ToString()";


"(1+1).ToString()" [](start = 48, length = 18)

nit: consider to add the comment why to add the NO-OP statement. #Closed

I've outlined the various feature improvements on the new fields/properties. I describe more about the parentScript below. I didn't do deep investigation into how or why adding code here works. I just noticed it in the testing. It didn't matter too much what the code I put here was. I seemed to get slightly better times if the logic here matched the first executed expression logic, such as doing arithmetic. But since the improvements were small and it entirely depends on the unique ForgeTree that each application uses, I don't think its worth any more investment to investigate and improve.

In reply to: 470868792 [](ancestors = 470868792)

zhengmu · 2020-08-14T21:20:05Z

Forge.TreeWalker/src/ExpressionExecutor.cs


        /// <summary>
-        /// Roslyn script options.
+        /// The ScriptCache is used to cache and re-use Roslyn scripts.
+        /// Scripts include the parentScript, as well as all unique expressions that get Executed.
        /// </summary>


[](start = 12, length = 10)

consider to add your comment in the code review description:
ScriptCache now caches Scripts that come from the parentScript.ContinueWith. ScriptCache still caches one Script per unique Expression, but these no longer get compiled. #Closed

Added more detail to ScriptCache description.

In reply to: 470870229 [](ancestors = 470870229)

zhengmu · 2020-08-14T21:21:02Z

/// </summary>

actually, consider to add your code review comments here, starting with "Roslyn Improvements Summary:....". Those perf numbers are good reference/baseline for future improvement if needed. #Closed

Refers to: Forge.TreeWalker/src/ExpressionExecutor.cs:24 in 9ed42b2. [](commit_id = 9ed42b2, deletion_comment = False)

TravisOnGit · 2020-08-14T22:01:29Z

/// </summary>

The history of this PR will not go away, so we can look back in the description to find the original notes if needed.
In general, I want the comments to reflect the current state of the code. I did include a brief note about the improvement numbers on parentScript, but I want to avoid adding too much historical info.

In reply to: 674277179 [](ancestors = 674277179)

Refers to: Forge.TreeWalker/src/ExpressionExecutor.cs:24 in 9ed42b2. [](commit_id = 9ed42b2, deletion_comment = False)

zhengmu

Roslyn Improvements. Memory consuption down from 75MB per unique expr…

9ed42b2

…ession to a flat 25MB plus 0.3MB~ per unique expression. Running time of first execution down from 2 seconds for each unique expression to a flat 2 seconds plus 15ms per unique expression.

abhayketkar approved these changes Aug 14, 2020

View reviewed changes

zhengmu reviewed Aug 14, 2020

View reviewed changes

Adding a few more details to summaries for future reference.

8047f3f

zhengmu approved these changes Aug 14, 2020

View reviewed changes

TravisOnGit merged commit ff083e0 into master Aug 14, 2020

TravisOnGit deleted the feature/RoslynImprovements branch December 9, 2020 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major Roslyn Improvements to Memory and Running Time #25

Major Roslyn Improvements to Memory and Running Time #25

TravisOnGit commented Aug 14, 2020

abhayketkar left a comment

zhengmu Aug 14, 2020 •

edited

TravisOnGit Aug 14, 2020

zhengmu Aug 14, 2020 •

edited

TravisOnGit Aug 14, 2020

zhengmu Aug 14, 2020 •

edited

TravisOnGit Aug 14, 2020

zhengmu commented Aug 14, 2020 •

edited

TravisOnGit commented Aug 14, 2020

zhengmu left a comment

Major Roslyn Improvements to Memory and Running Time #25

Major Roslyn Improvements to Memory and Running Time #25

Conversation

TravisOnGit commented Aug 14, 2020

abhayketkar left a comment

Choose a reason for hiding this comment

zhengmu Aug 14, 2020 • edited

Choose a reason for hiding this comment

TravisOnGit Aug 14, 2020

Choose a reason for hiding this comment

zhengmu Aug 14, 2020 • edited

Choose a reason for hiding this comment

TravisOnGit Aug 14, 2020

Choose a reason for hiding this comment

zhengmu Aug 14, 2020 • edited

Choose a reason for hiding this comment

TravisOnGit Aug 14, 2020

Choose a reason for hiding this comment

zhengmu commented Aug 14, 2020 • edited

TravisOnGit commented Aug 14, 2020

zhengmu left a comment

Choose a reason for hiding this comment

zhengmu Aug 14, 2020 •

edited

zhengmu Aug 14, 2020 •

edited

zhengmu Aug 14, 2020 •

edited

zhengmu commented Aug 14, 2020 •

edited