Bug 167614 - The sampling profile should have an option to sample from C frames.
Summary: The sampling profile should have an option to sample from C frames.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-30 16:46 PST by Keith Miller
Modified: 2017-02-01 17:25 PST (History)
4 users (show)

See Also:


Attachments
WIP (11.48 KB, patch)
2017-01-30 16:51 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2017-01-31 09:39 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2017-01-31 11:14 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2017-01-31 11:14 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (15.18 KB, patch)
2017-02-01 16:03 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (15.18 KB, patch)
2017-02-01 16:04 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (15.02 KB, patch)
2017-02-01 16:06 PST, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2017-01-30 16:46:42 PST
The sampling profile should have an option to sample from C frames.
Comment 1 Keith Miller 2017-01-30 16:51:25 PST
Created attachment 300160 [details]
WIP
Comment 2 Keith Miller 2017-01-31 09:39:39 PST
Created attachment 300224 [details]
Patch
Comment 3 Keith Miller 2017-01-31 11:14:00 PST
Created attachment 300238 [details]
Patch
Comment 4 Keith Miller 2017-01-31 11:14:50 PST
Created attachment 300239 [details]
Patch
Comment 5 Saam Barati 2017-01-31 11:53:15 PST
Comment on attachment 300239 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:228
> +            resetAtMachineFrame();

I think you may want your own version of resetAtMachineFrame that also calls into the parent's version. The version here should probably veryify that:
 "isValidFramePointer(m_machineFrame)"

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:608
> +                stackTrace.frames.last().frameType = FrameType::Host;

Why not add a new frame type?
You can make the various functions that look at frame type for the inspector just skip over C frames for now.

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:742
> +#if OS(DARWIN)
> +        if (Options::sampleCCode()) {
> +            const char* mangledName = nullptr;
> +            const char* cxaDemangled = nullptr;
> +            Dl_info info;
> +            if (dladdr(cCodePC, &info) && info.dli_sname)
> +                mangledName = info.dli_sname;
> +            if (mangledName) {
> +                cxaDemangled = abi::__cxa_demangle(mangledName, 0, 0, 0);
> +                return String(cxaDemangled ? cxaDemangled : mangledName);
> +            }
> +        }
> +#endif

Ditto about the FrameType.
Comment 6 Keith Miller 2017-02-01 16:03:57 PST
Created attachment 300362 [details]
Patch for landing
Comment 7 Keith Miller 2017-02-01 16:04:22 PST
Created attachment 300364 [details]
Patch for landing
Comment 8 Keith Miller 2017-02-01 16:06:52 PST
Created attachment 300365 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2017-02-01 17:25:03 PST
Comment on attachment 300365 [details]
Patch for landing

Clearing flags on attachment: 300365

Committed r211542: <http://trac.webkit.org/changeset/211542>
Comment 10 WebKit Commit Bot 2017-02-01 17:25:07 PST
All reviewed patches have been landed.  Closing bug.