Experimental Stackguard #1566
Experimental Stackguard #1566
Conversation
windows error fixed Exception finetuned with Exception helper tests updated for latest stackgruad
|
Hi @lahma, I hope you are well. I'm not sure if the improvement I made will work, but it can prevent stack overflow. Can you review it when you are available? Maybe you can suggest something to increase performance and decrease memory usage. |
There was a problem hiding this comment.
Cool stuff indeed, I wasn't aware of this StackGuard until you introduced it, thanks! I've added some preliminary comments. I don't object this idea. Probably need to run some performance tests of my own too - the code paths aren't utilized with Jint anyway so it's more like for bigger recursion. Maybe separate test case proving that 10 000 can be achieved?
| @@ -9,21 +9,12 @@ public class EngineLimitTests | |||
| [Fact] | |||
| public void ShouldAllowReasonableCallStackDepth() | |||
| { | |||
| if (OperatingSystem.IsMacOS()) | |||
There was a problem hiding this comment.
I think for this test case we might want to disable the stack guard (value -1?) to ensure we don't start writing silly code - this is also a performance test when it comes to stack handling. So we would keep this test as-is expect for configuring engine with disabled stack guard.
Can of course give the script generation improvements (x starts with 1 and result checked).
There was a problem hiding this comment.
Disable options (-1) make sense. I add disable option and also kept the actual test case as-is.
| new Engine(options => options.Constraints.MaxExecutionStackCount = 1000) | ||
| .SetValue("assert", new Action<bool>(Assert.True)) | ||
| .SetValue("log", new Action<object>(Console.WriteLine)) | ||
| .Evaluate(@" |
There was a problem hiding this comment.
maybe no console write lines, they just make test output bad - maybe just return needed data from Evaluate
There was a problem hiding this comment.
log removed.
I used it for testing locally and did not expect to effect test output :)
I updated the test to 10 000 but it takes more time for unit test in my local machine takes 33sec. It is may not ideal for github actions |
OK I might have been overly ambitious, maybe test case should have |
| /// <summary> | ||
| /// Maximum recursion stack count, defaults to -1 (as-is dotnet stacktrace). | ||
| /// </summary> | ||
| public int MaxExecutionStackCount { get; set; } = StackGuard.Disabled; |
There was a problem hiding this comment.
I think the old comment had valuable info 14000 which comes from Chrome and V8 based engines (ClearScript) that can handle 13955. So might be good to indicate that option will allow more nested calls with slight performance / stack trace readability drawback. (after hitting the engine's own limit), The old info gave reasonable limit for expectations without people resorting to int.MaxValue.
There was a problem hiding this comment.
I updated it as remarks. If you have another suggestion, please share it with me. It will be a pleasure for me to improve it.
|
Looking good, I gave a few more comments for your consideration, sorry for the delay. |
Extend Stack overflow #1159
Jint.Benchmark.SunSpiderBenchmark