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> 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 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 >>> >>> <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 count = 10150 > ....split(…).length = 11 (because Chromes starts e.stack with a line > "RangeError: Maximum call stack size exceeded”) > e.stack lines according to editor = 10 frames > > 2. Firefox > test reported reentry count = 222044 > ....split(…).length = 129 (probably because there’s an extra newline in > there somewhere) > e.stack lines according to editor = 128 frames > > 3. Safari > test reported reentry count = 31701 > ....split(…).length = 31722 (I don’t know why there’s a 21 frame > discrepancy here. I’ll debug this later) > e.stack lines according to editor = ??? frames (WebInspector hangs every > time I try to scroll in it, let alone let me highlight and copy the stack > trace. So I gave up) > > Assuming the test function frame is not significantly different in size for > all browsers, it looks like: > 1. Chrome uses a much smaller stack (about 1/3 of our stack). > 2. Firefox uses a much larger stack (possibly the full machine stack), but > caps its Error.stack to just 128 frames (possibly a hardcoded limit). > > Mark > > >> >> - Maciej >> >>> >>> Does anyone object to us adopting Error.stackTraceLimit and setting the >>> default to 10 to match Chrome? >>> >>> Mark >>> >>> >>> >>>> On Mar 16, 2017, at 11:29 PM, Geoffrey Garen <gga...@apple.com >>>> <mailto:gga...@apple.com>> wrote: >>>> >>>> Can you be more specific about the motivation here? >>>> >>>> Do we have any motivating examples that will tell us wether time+memory >>>> were unacceptable before this change, or are acceptable after this change? >>>> >>>> In our motivating examples, does Safari use more time+memory than other >>>> browsers? If so, how large of a stack do other browsers capture? >>>> >>>> We already limit the size of the JavaScript stack to avoid performance >>>> problems like the ones you mention in many other contexts. Why is that >>>> limit not sufficient? >>>> >>>> Did you consider implementing Chrome’s Error.stackTraceLimit behavior? >>>> >>>> Geoff >>>> >>>>> On Mar 16, 2017, at 10:09 PM, Mark Lam <mark....@apple.com >>>>> <mailto:mark....@apple.com>> wrote: >>>>> >>>>> Hi folks, >>>>> >>>>> Currently, if we have an exception stack that is incredibly deep >>>>> (especially for a StackOverflowError), JSC may end up thrashing memory >>>>> just to capture the large stack trace in memory. This is bad for many >>>>> reasons: >>>>> >>>>> 1. the captured stack will take a lot of memory. >>>>> 2. capturing the stack may take a long time (due to memory thrashing) and >>>>> makes for a bad user experience. >>>>> 3. if memory availability is low, capturing such a large stack may result >>>>> in an OutOfMemoryError being thrown in its place. >>>>> The OutOfMemoryError thrown there will also have the same problem with >>>>> capturing such a large stack. >>>>> 4. most of the time, no one will look at the captured Error.stack anyway. >>>>> >>>>> Since there isn’t a standard on what we really need to capture for >>>>> Error.stack, I propose that we limit how much stack we capture to a >>>>> practical size. How about an Error.stack that consists of (1) the top N >>>>> frames, (2) an ellipses, and (3) the bottom M frames? If the number of >>>>> frames on the stack at the time of capture is less or equal to than N + >>>>> M frames, then Error.stack will just show the whole stack with no >>>>> ellipses. For example, if N is 4 and M is 2, the captured stack will >>>>> look something like this: >>>>> >>>>> foo10001 >>>>> foo10000 >>>>> foo9999 >>>>> foo9998 >>>>> … >>>>> foo1 >>>>> foo0 >>>>> >>>>> If we pick a sufficient large number for N and M (I suggest 100 each), I >>>>> think this should provide sufficient context for debugging uses of >>>>> Error.stack, while keeping an upper bound on how much memory and time we >>>>> throw at capturing the exception stack. >>>>> >>>>> My plan for implementing this is: >>>>> 1. change Exception::finishCreation() to only capture the N and M frames, >>>>> plus possibly 1 ellipses placeholder in the between them. >>>>> 2. change all clients of Exception::stack() to be able to recognize and >>>>> render the ellipses. >>>>> >>>>> Does anyone object to doing this or have a compelling reason why this >>>>> should not be done? >>>>> >>>>> Thanks. >>>>> >>>>> Mark >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> webkit-dev mailing list >>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >>>> >>> >>> _______________________________________________ >>> webkit-dev mailing list >>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> > _______________________________________________ > webkit-dev mailing list > webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> > https://lists.webkit.org/mailman/listinfo/webkit-dev > <https://lists.webkit.org/mailman/listinfo/webkit-dev>
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
