[wasm][debugger] Improvement of memory consumption while Evaluating Expressions #61470
Conversation
|
Tagging subscribers to this area: @thaystg Issue DetailsRemoving the usage of CSharpCompilation which was increasing the memory consumption and using CSharpScript.
|
| case "void": | ||
| return "object"; | ||
| case "boolean": |
There was a problem hiding this comment.
This line is testing it
There was a problem hiding this comment.
Ah, and this was being handled by default: return value.GetType().FullName; earlier? Why the additional case then?
There was a problem hiding this comment.
Because I did this change:
And this change was needed because when the variable was being declared and used it was creating like this: var exampleVar4893742 = True; and the first letter of the True/False with uppercase throws a compilation error.
There was a problem hiding this comment.
ok, that makes sense. IIUC, ConvertJSToCSharpType is being called once to get the value, and then GetTypeFullName also calls it. Instead, I think it would make sense for ConvertJSToCSharpType to return a (string type, string value), since the type that we want to use in the final expression is tied closely to the "value" that we return.
And the method can be renamed to something like ConvertJSToCSharpVariable.
Co-authored-by: Larry Ewing <lewing@microsoft.com>
thaystg
added
the
NO-MERGE
| valueRet = "Newtonsoft.Json.Linq.JObject.FromObject(new {" | ||
| + $"type = \"{type}\"" | ||
| + $", description = \"{variable["description"].Value<string>()}\"" | ||
| + $", className = \"{variable["className"].Value<string>()}\"" | ||
| + (subType != null ? $", subtype = \"{subType}\"" : "") | ||
| + (objectId != null ? $", objectId = \"{objectId}\"" : "") | ||
| + "})"; | ||
| typeRet = "object"; | ||
| break; | ||
| case "void": | ||
| return null; | ||
| } | ||
| throw new Exception($"Evaluate of this datatype {type} not implemented yet");//, "Unsupported"); | ||
| } | ||
|
|
||
| private string GetTypeFullName(JToken variable) | ||
| { | ||
| string type = variable["type"].ToString(); | ||
| string subType = variable["subtype"]?.Value<string>(); | ||
| object value = ConvertJSToCSharpType(variable); | ||
|
|
||
| switch (type) | ||
| { | ||
| case "object": | ||
| { | ||
| if (subType == "null") | ||
| return variable["className"].Value<string>(); | ||
| else | ||
| return "object"; | ||
| } | ||
| case "void": | ||
| return "object"; | ||
| valueRet = "Newtonsoft.Json.Linq.JObject.FromObject(new {" | ||
| + $"type = \"object\"," | ||
| + $"description = \"object\"," | ||
| + $"className = \"object\"," | ||
| + $"subtype = \"null\"" | ||
| + "})"; | ||
| typeRet = "object"; | ||
| break; |
There was a problem hiding this comment.
I'm not sure I understand these two.
- why do we need to use JObjects in the script?
case "object":- would it be better to useNewtonsoft.Json.Linq.JObject.Parse(..)and pass the escaped string forvariable?case "void":- why is this needed? And what does thevariablelook like here? And do we have a test for this?
There was a problem hiding this comment.
- Because that is how we return simple variable values, so when it's something that we really need to execute we also keep the same way of returning the value.
- I think it's faster to not parse the string? once this is already parsed we don't need to parse it again?
- Yes, the tests evaluating method that return void.
| var result = await CSharpScript.EvaluateAsync( | ||
| string.Join("\n", findVarNMethodCall.variableDefinitions) + "\nreturn " + syntaxTree.ToString(), | ||
| ScriptOptions.Default.WithReferences( | ||
| typeof(object).Assembly, | ||
| typeof(Enumerable).Assembly, | ||
| typeof(JObject).Assembly | ||
| ), | ||
| cancellationToken: token); |
There was a problem hiding this comment.
If we specifically catch CompilationErrorException here, can we surface the error to VS? or maybe just return the exception message like on line 400.
There was a problem hiding this comment.
Do I understand it correctly that if the expression itself throws an exception, then we return the message on line 400? Can we detect this specific case of the expression itself throwing, and return a different message?
There was a problem hiding this comment.
If we specifically catch
CompilationErrorExceptionhere, can we surface the error to VS? or maybe just return the exception message like on line 400.
IIUC, this would be thrown if our constructed script fails to compile, so maybe the message doesn't need to be shown to the user.
There was a problem hiding this comment.
Do I understand it correctly that if the expression itself throws an exception, then we return the message on line 400? Can we detect this specific case of the expression itself throwing, and return a different message?
Looking at the code there is some handling for exceptions, though specifically System.Exception, and that then returns the message.
Though looking at it, this.PropertyThrowException correctly returns the exception message, but this.ParmToTestObjException.MyMethod() doesn't.
This doesn't need to be handled for this PR though.
thaystg
removed
the
NO-MERGE
| @@ -7,6 +7,7 @@ | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="3.7.0" /> | |||
There was a problem hiding this comment.
For future PR, we should consider using the common version (more than one!) property for this, being used in other places in the repo.
ghost
locked as resolved and limited conversation to collaborators
Removing the usage of CSharpCompilation which was increasing the memory consumption and using CSharpScript.