Bug 169454 – Implement a StackTrace utility object that can capture stack traces for debugging.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 304007 [details]
proposed patch.

[Mark Lam]

Comment on attachment 304007 [details]
proposed patch.

Need to add file to CMakeLists.txt.

[Mark Lam]

Created attachment 304009 [details]
proposed patch.

[Michael Saboff]

Comment on attachment 304009 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=304009&action=review

r-
Please address the sizeof issue in StackTrace::instanceSize()

> Source/JavaScriptCore/ChangeLog:19
> +        In addition, not printing stack traces all the time also allows us to pollute
> +        stdout with tons of stack traces that may be irrelevant.

I don't understand "not printing" with the rest of the sentence.

> Source/JavaScriptCore/ChangeLog:31
> +            // Capture a stack trace of the top 10 frames.
> +            StackTrace* trace = StackTrace::captureStackTrace(10);
> +            // Print the trace.
> +            dataLog(*trace);
> +            // Release the trace.
> +            delete trace;

Could we change to a model where we ref count the StackTrace?

> Source/JavaScriptCore/tools/StackTrace.cpp:52
> +    return sizeof(StackTrace) + (capacity - 1);

sizeof() returns the size in bytes.  Don't you need the sizeof(void*) * (capacity - 1)?  It would probably be even better to have a StackTraceFrame typedef that is used for storing the type and computing the size needed per frame.

> Source/JavaScriptCore/tools/StackTrace.h:58
> +    // We structure the top fields this way because the underlying stack capture
> +    // facility will capture from the top of the stack, and we'll need to skip the
> +    // top 2 frame which is of no interest. Setting up the fields layout this way
> +    // allows us to capture the stack in place and minimize space wastage.
> +    union {
> +        struct {
> +            int m_size { 0 };
> +            int m_capacity;
> +        };
> +        struct {
> +            void* m_skippedFrame0;
> +            void* m_skippedFrame1;
> +        };

This seems a little overkill for saving the size of two ints.

[Mark Lam]

Comment on attachment 304009 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=304009&action=review

>> Source/JavaScriptCore/ChangeLog:19
>> +        stdout with tons of stack traces that may be irrelevant.
> 
> I don't understand "not printing" with the rest of the sentence.

I had a typo there.  I meant to say: "not printing stack traces all the time also allows us to avoid polluting stdout with tons of stack traces that may be irrelevant."

>> Source/JavaScriptCore/ChangeLog:31
>> +            delete trace;
> 
> Could we change to a model where we ref count the StackTrace?

We can but I think that is not meaningful.  One example use case I'm trying to satisfy (elaborating on what I described above) is that I would like to capture a stack trace at every allocation point and stash the trace away with the allocated object.  If I see a corruption related to that object later, I can dump its stack trace to know where it came from.  As a result, I would want the allocation and capturing of the stack trace to be as fast as possible, as well as take as little memory as possible.

Ref counting does not help my use case, and can actually hurt.  What do you have in mind for a use case that would necessitate ref counting it?

>> Source/JavaScriptCore/tools/StackTrace.cpp:52
>> +    return sizeof(StackTrace) + (capacity - 1);
> 
> sizeof() returns the size in bytes.  Don't you need the sizeof(void*) * (capacity - 1)?  It would probably be even better to have a StackTraceFrame typedef that is used for storing the type and computing the size needed per frame.

Eeek, you're right.  Will fix.

>> Source/JavaScriptCore/tools/StackTrace.h:58
>> +        };
> 
> This seems a little overkill for saving the size of two ints.

In my use case, I may be allocating one of the stack traces for every object (or every Nth object) ever allocated.  In that use case, every byte counts and this.  I'd like to keep this.

[Mark Lam]

Created attachment 304012 [details]
proposed patch: fixed size calculation and ChangeLog.

[Michael Saboff]

(In reply to comment #5)
> Comment on attachment 304009 [details]

> >> Source/JavaScriptCore/ChangeLog:31
> >> +            delete trace;
> > 
> > Could we change to a model where we ref count the StackTrace?
> 
> We can but I think that is not meaningful.  One example use case I'm trying
> to satisfy (elaborating on what I described above) is that I would like to
> capture a stack trace at every allocation point and stash the trace away
> with the allocated object.  If I see a corruption related to that object
> later, I can dump its stack trace to know where it came from.  As a result,
> I would want the allocation and capturing of the stack trace to be as fast
> as possible, as well as take as little memory as possible.
> 
> Ref counting does not help my use case, and can actually hurt.  What do you
> have in mind for a use case that would necessitate ref counting it?

I think stashing a stack trace with every allocated object makes my case.  With a raw pointer, you need to add a delete when the object is destroyed.  With a ref ptr, the stack trace will be cleaned up automatically for C++ allocated objects.

Ref ptrs are not that expensive relative to actually doing the stack trace.

[Michael Saboff]

Comment on attachment 304012 [details]
proposed patch: fixed size calculation and ChangeLog.

View in context: https://bugs.webkit.org/attachment.cgi?id=304012&action=review

r=me

> Source/JavaScriptCore/tools/StackTrace.cpp:52
> +    return sizeof(StackTrace) + (capacity - 1) * sizeof(void*);

I still prefer a typedef instead of void*.

[Mark Lam]

(In reply to comment #7)
> (In reply to comment #5)
> > Comment on attachment 304009 [details]
> 
> > >> Source/JavaScriptCore/ChangeLog:31
> > >> +            delete trace;
> > > 
> > > Could we change to a model where we ref count the StackTrace?
> > 
> > We can but I think that is not meaningful.  One example use case I'm trying
> > to satisfy (elaborating on what I described above) is that I would like to
> > capture a stack trace at every allocation point and stash the trace away
> > with the allocated object.  If I see a corruption related to that object
> > later, I can dump its stack trace to know where it came from.  As a result,
> > I would want the allocation and capturing of the stack trace to be as fast
> > as possible, as well as take as little memory as possible.
> > 
> > Ref counting does not help my use case, and can actually hurt.  What do you
> > have in mind for a use case that would necessitate ref counting it?
> 
> I think stashing a stack trace with every allocated object makes my case. 
> With a raw pointer, you need to add a delete when the object is destroyed. 
> With a ref ptr, the stack trace will be cleaned up automatically for C++
> allocated objects.
> 
> Ref ptrs are not that expensive relative to actually doing the stack trace.

Don't need a RefPtr for that.  The owner can use a std::unique_ptr which addresses what you want.

[Mark Lam]

Created attachment 304014 [details]
Patch for landing.

Thanks for the review.

Not a big deal, but I stuck with the void* because that's what WTFGetBacktrace() wants.  Per our offline discussion, I also moved the capacity fixup from StackTrace::instanceSize() to StackTrace::captureStackTrace() so that the value stored in m_capacity reflects the fixed up capacity.  Instead, in StackTrace::instanceSize(), I assert that the capacity is sane.  I also changed the ChangeLog to use std::unique_ptr to suggest the better idiom for holding a StackTrace.

[Mark Lam]

Landed in r213695: <http://trac.webkit.org/r213695>.

[Ryan Haddad]

(In reply to comment #11)
> Landed in r213695: <http://trac.webkit.org/r213695>.

This change broke the Windows build:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/84680/steps/compile-webkit/logs/stdio

[Ryan Haddad]

Reverted r213695 for reason:

This change broke the Windows build.

Committed r213702: <http://trac.webkit.org/changeset/213702>

[Mark Lam]

Created attachment 304053 [details]
Patch for re-landing.

Let's see how the windows EWS likes this.

[Mark Lam]

The win EWS bot is happy.  Re-landed in r213718: <http://trac.webkit.org/r213718>.