Re: [webkit-dev] Proposal to limit the size of the captured exception stack

2017-03-29 Thread Matt Baker
> On Mar 28, 2017, at 9:55 PM, Mark Lam <mark@apple.com> wrote:
> 
> Matt,
> I tested this on Firefox and Chrome and saw that Chrome captures up to 200 
> frames.  I don’t see Firefox capturing frames at all when not throwing an 
> Error.  Were you looking at Error.stack for Firefox when you came up with the 
> 128 frames number?  Maybe there’s a Firebug option I’m not familiar with?

The 128 frames is what I observed in Firefox’s console after loading a page 
which runs a script recursively in order to trigger a stack overflow. Enabling 
“Pause on Exceptions” in Firefox didn’t cause the debugger to pause when 
throwing the error (same as Chrome) so I’m not sure how to interact with the 
Error object.

> Geoff,
> It just occurred to me that the developer does have one recourse: he/she can 
> use break on exception thrown / on uncaught exception, and inspect the full 
> stack via the debugger.
> 
> Mark
> 
> 
>> On Mar 28, 2017, at 9:08 PM, Mark Lam <mark@apple.com 
>> <mailto:mark@apple.com>> wrote:
>> 
>> Currently, there’s no way for the developer to change this.  We can 
>> certainly make it an option that the Inspector can change if desired.
>> 
>> Mark
>> 
>> 
>>> On Mar 28, 2017, at 7:35 PM, Matt Baker <matthew_a_ba...@apple.com 
>>> <mailto:matthew_a_ba...@apple.com>> wrote:
>>> 
>>>> On Mar 28, 2017, at 4:23 PM, Geoffrey Garen <gga...@apple.com 
>>>> <mailto:gga...@apple.com>> wrote:
>>>> 
>>>> Does the separate exceptionStackTraceLimit mean that if a developer gets a 
>>>> truncated stack trace in the Web Inspector, there’s no way for the 
>>>> developer to remedy that? Is that what other browsers’ developer tools do?
>>> 
>>> FireFox and Chrome show console entires with exception stack traces with 
>>> 128 and 200 frames (respectively).
>>> 
>>>> 
>>>> Geoff
>>>> 
>>>>> On Mar 28, 2017, at 4:09 PM, Mark Lam <mark@apple.com 
>>>>> <mailto:mark@apple.com>> wrote:
>>>>> 
>>>>> To follow up, I’ve implemented the change in r214289: <http://trac.webkit 
>>>>> <http://trac.webkit/>.org/r214289>.  Error.stackTraceLimit is now 100.  I 
>>>>> also implemented a separate exceptionStackTraceLimit for stack traces 
>>>>> captured at the time of throwing a value (not to be confused with 
>>>>> Error.stack which is captured at the time of instantiation of the Error 
>>>>> object).  exceptionStackTraceLimit is also limited to 100 by default.
>>>>> 
>>>>> Mark
>>>>> 
>>>>> 
>>>>>> On Mar 17, 2017, at 1:04 PM, Mark Lam <mark@apple.com 
>>>>>> <mailto:mark@apple.com>> wrote:
>>>>>> 
>>>>>> @Geoff, my testing shows that we can do 200 frames and still perform 
>>>>>> well (~1 second to console.log Error.stack).  Base on what we at 
>>>>>> present, I think 100 is a good round number to use as our default 
>>>>>> stackTraceLimit.
>>>>>> 
>>>>>>> On Mar 17, 2017, at 11:40 AM, Maciej Stachowiak <m...@apple.com 
>>>>>>> <mailto:m...@apple.com>> wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>> On Mar 17, 2017, at 11:09 AM, Mark Lam <mark@apple.com 
>>>>>>>> <mailto:mark@apple.com>> wrote:
>>>>>>>> 
>>>>>>>> Thanks for the reminder to back observations up with data.  I was 
>>>>>>>> previously running some tests that throws StackOverflowErrors a lot 
>>>>>>>> (which tainted my perspective), and I made a hasty conclusion which 
>>>>>>>> isn’t good.  Anyway, here’s the data using an instrumented VM to take 
>>>>>>>> some measurements and a simple test program that recurses forever to 
>>>>>>>> throw a StackOverflowError (run on a MacPro):
>>>>>>>> 
>>>>>>>> 1. For a release build of jsc shell:
>>>>>>>> Time to capture exception stack = 0.002807 sec
>>>>>>>> Number of stack frames captured = 31722
>>>>>>>> sizeof StackFrame = 24
>>>>>>>> total memory consumed = ~761328 bytes.
>>>>>>>> 
>>>>>>>> 2. For a d

Re: [webkit-dev] Proposal to limit the size of the captured exception stack

2017-03-28 Thread Matt Baker
> On Mar 28, 2017, at 4:23 PM, Geoffrey Garen  wrote:
> 
> Does the separate exceptionStackTraceLimit mean that if a developer gets a 
> truncated stack trace in the Web Inspector, there’s no way for the developer 
> to remedy that? Is that what other browsers’ developer tools do?

FireFox and Chrome show console entires with exception stack traces with 128 
and 200 frames (respectively).

> 
> Geoff
> 
>> On Mar 28, 2017, at 4:09 PM, Mark Lam > > wrote:
>> 
>> To follow up, I’ve implemented the change in r214289: > .org/r214289>.  Error.stackTraceLimit is now 100.  I 
>> also implemented a separate exceptionStackTraceLimit for stack traces 
>> captured at the time of throwing a value (not to be confused with 
>> Error.stack which is captured at the time of instantiation of the Error 
>> object).  exceptionStackTraceLimit is also limited to 100 by default.
>> 
>> Mark
>> 
>> 
>>> On Mar 17, 2017, at 1:04 PM, Mark Lam >> > wrote:
>>> 
>>> @Geoff, my testing shows that we can do 200 frames and still perform well 
>>> (~1 second to console.log Error.stack).  Base on what we at present, I 
>>> think 100 is a good round number to use as our default stackTraceLimit.
>>> 
 On Mar 17, 2017, at 11:40 AM, Maciej Stachowiak > wrote:
 
> 
> On Mar 17, 2017, at 11:09 AM, Mark Lam  > wrote:
> 
> Thanks for the reminder to back observations up with data.  I was 
> previously running some tests that throws StackOverflowErrors a lot 
> (which tainted my perspective), and I made a hasty conclusion which isn’t 
> good.  Anyway, here’s the data using an instrumented VM to take some 
> measurements and a simple test program that recurses forever to throw a 
> StackOverflowError (run on a MacPro):
> 
> 1. For a release build of jsc shell:
> Time to capture exception stack = 0.002807 sec
> Number of stack frames captured = 31722
> sizeof StackFrame = 24
> total memory consumed = ~761328 bytes.
> 
> 2. For a debug build of jsc shell:
> Time to capture exception stack = 0.052107 sec
> Number of stack frames captured = 31688
> sizeof StackFrame = 24
> total memory consumed = ~760512 bytes.
> 
> So, regarding performance, I was wrong.  The amount of time taken to 
> capture the entire JS stack each time is insignificant.
> Regarding memory usage, ~760K is not so good, but maybe it’s acceptable.
> 
> Comparing browsers with their respective inspectors open:
> 
> 1. Chrome
> number of frames captured: 10
> length of e.stack string: 824 chars
> time to console.log e.stack: 0.27 seconds
> 
> 2. Firefox
> number of frames captured: 129
> length of e.stack string: 8831 chars
> time to console.log e.stack: 0.93 seconds
> 
> 3. Safari
> number of frames captured: 31722
> length of e.stack string: 218821 chars
> time to console.log e.stack: 50.8 seconds
> 
> 4. Safari (with error.stack shrunk to 201 frames at time of capture to 
> simulate my proposal)
> number of frames captured: 201
> length of e.stack string: 13868 chars
> time to console.log e.stack: 1 second
> 
> With my proposal, the experience of printing Error.stack drops from 50.8 
> seconds to about 1 second.  The memory used for capturing the stack also 
> drops from ~760K to 5K.
> 
> I wasn’t aware of the Error.stackTraceLimit, but that does sound like a 
> better solution than my proposal since it gives developers the ability to 
> capture more stack frames if they need it.  Chrome’s default 
> Error.stackTraceLimit appears to be 10.  MS appears to support it as well 
> and defaults to 10 
> (https://docs.microsoft.com/en-us/scripting/javascript/reference/stacktracelimit-property-error-javascript
>  
> ).
>   Firefox does now.
 
 Out of curiosity: Why does Firefox capture 129 frames instead of 31722 in 
 this case? Do they have a hardcoded limit?
>>> 
>>> Actually, my previous frame counts are a bit off.  I was using 
>>> e.stack.split(/\r\n|\r|\n/).length as the frame count.  Below, I just copy 
>>> the console.log dump into an editor and take the line count from there as 
>>> the frame count instead.  The result of that string.split appears to be a 
>>> bit off from the actual frames printed by console.log. 
>>> 
>>> I also modified my recursing test function to console.log the re-entry 
>>> count on entry and this is what I saw:
>>> 
>>> 1. Chrome
>>> test reported reentry