Bug 152629

Summary: JSC Sampling Profiler: Come up with a (program counter => CodeOrigin) mapping
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 151713, 154391    
Bug Blocks: 153508, 153455, 153663    
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
WIP
none
more cleanup
none
more cleanup
none
make findPC find more PCs
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch fpizlo: review+

Description Saam Barati 2015-12-31 15:40:28 PST
Hopefully we can get away with always storing this by finding a nice way to compress this information. This shouldn't be too difficult.

After thinking about the uses for this mapping, we should only need to consult this mapping for
the top frame in a stack trace. All other parent frames should have the call site index in an
accurate state on the stack which means we'll just be able to consult that CallSiteIndex to
retrieve the CodeOrigin.

The top frame, though, might not have the CallSiteIndex be in an accurate state.
This could happen for a number of reasons, for example: let's say we make a call
from inside an inlined function, but then pause the process after that call took
place, and before any other calls are made, and the PC is now at code that isn't from the
same inlined function. The CallSiteIndex will still show us executing in the inlined 
function, which isn't correct. There are also parts of the function, in the prologue for example,
where we could just have garbage bits for the CallSiteIndex. Having a PC=>CodeOrigin map would solve
these problems. I'm sure there are more situations it will help with, too.
There will probably be times, though, where the PC=>CodeOrigin map won't help us, but we're still
able to take a valid stack trace, and in those situations, we will probably have to forfeit
getting high fidelity location information and settle for just knowing which function
we're executing inside.
Comment 1 Saam Barati 2016-01-19 16:57:42 PST
Created attachment 269310 [details]
WIP

it's a start.
Comment 2 Saam Barati 2016-01-21 17:01:17 PST
Created attachment 269522 [details]
WIP
Comment 3 Saam Barati 2016-01-21 18:27:18 PST
Created attachment 269532 [details]
WIP

now accounting for inlined frames at the top of the stack in a stack traces.
Comment 4 Saam Barati 2016-01-25 16:40:49 PST
Created attachment 269814 [details]
WIP

I think it's almost done.
Comment 5 Saam Barati 2016-01-25 18:55:46 PST
Created attachment 269839 [details]
more cleanup
Comment 6 Saam Barati 2016-01-25 19:18:56 PST
Created attachment 269843 [details]
more cleanup
Comment 7 Saam Barati 2016-01-26 13:35:08 PST
Created attachment 269911 [details]
make findPC find more PCs
Comment 8 Saam Barati 2016-01-26 13:50:56 PST
Created attachment 269913 [details]
patch
Comment 9 WebKit Commit Bot 2016-01-26 13:52:19 PST
Attachment 269913 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:33:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:61:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JIT.h:50:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:54:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:178:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:126:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:155:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:159:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:175:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:267:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 14 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Saam Barati 2016-01-26 14:24:30 PST
Created attachment 269921 [details]
patch

hopefully fix builds.
fix some style issues
Comment 11 Saam Barati 2016-01-26 14:33:40 PST
Created attachment 269924 [details]
patch

rebased
Comment 12 WebKit Commit Bot 2016-01-26 14:35:42 PST
Attachment 269924 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:61:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:126:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:155:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:159:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:175:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:267:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Saam Barati 2016-01-26 14:49:58 PST
Comment on attachment 269924 [details]
patch

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

> Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp:46
> +    m_globalObject.vm().setShouldBuildPCToCodeOriginMapping();

I'm moving this into Debugger::attach().
Comment 14 Saam Barati 2016-01-26 14:52:37 PST
Comment on attachment 269924 [details]
patch

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

> Source/JavaScriptCore/b3/B3PCToOriginMap.h:58
> +            if (m_ranges.last().origin == origin || !origin)
> +                return;

this should be removed and is locally
Comment 15 Saam Barati 2016-01-26 14:59:37 PST
Comment on attachment 269924 [details]
patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4290
> +#if ENABLE(JIT)

I'm going to make PCToCodeOrigin map only build under ENABLE(JIT)
Comment 16 Saam Barati 2016-01-26 16:28:41 PST
Created attachment 269947 [details]
patch
Comment 17 WebKit Commit Bot 2016-01-26 16:31:31 PST
Attachment 269947 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:59:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:157:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:161:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:177:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:269:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Keith Miller 2016-01-27 17:48:21 PST
I haven't looked at the patch fully but what about adding an option that always turns this on? I think it might be useful for debugging. Obviously, the option can come in a later patch.
Comment 19 Saam Barati 2016-01-27 18:00:39 PST
(In reply to comment #18)
> I haven't looked at the patch fully but what about adding an option that
> always turns this on? I think it might be useful for debugging. Obviously,
> the option can come in a later patch.

Yeah sure I'll add one.
It'd be even better if that option created a global table
so you could always go from:
 (VM, PC) => CodeBlock.
Comment 20 Saam Barati 2016-01-28 21:53:27 PST
Comment on attachment 269947 [details]
patch

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

> Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:81
> +class DeltaCompressionBuilder {
> +public:
> +    DeltaCompressionBuilder(size_t maxSize)
> +        : m_offset(0)
> +        , m_maxSize(maxSize)
> +    {
> +        m_buffer = static_cast<uint8_t*>(fastMalloc(m_maxSize));
> +    }
> +
> +    template <typename T>
> +    void write(T item)
> +    {
> +        RELEASE_ASSERT(m_offset + sizeof(T) <= m_maxSize);
> +        *bitwise_cast<T*>(m_buffer + m_offset) = item;
> +        m_offset += sizeof(T);
> +    }
> +
> +    uint8_t* m_buffer; 
> +    size_t m_offset;
> +    size_t m_maxSize;
> +};
> +
> +class DeltaCompresseionReader {
> +public:
> +    DeltaCompresseionReader(uint8_t* buffer, size_t size)
> +        : m_buffer(buffer)
> +        , m_size(size)
> +        , m_offset(0)
> +    { }
> +
> +    template <typename T>
> +    T read()
> +    {
> +        RELEASE_ASSERT(m_offset + sizeof(T) <= m_size);
> +        T result = *bitwise_cast<T*>(m_buffer + m_offset);
> +        m_offset += sizeof(T);
> +        return result;
> +    }
> +
> +private:
> +    uint8_t* m_buffer;
> +    size_t m_size;
> +    size_t m_offset;

After speaking w/ Andreas today, I realize that this
probably won't play nice w/ ARM alignment requirements.
I'm going to make the necessary changes to make this work.
Comment 21 Saam Barati 2016-01-29 13:09:29 PST
Created attachment 270246 [details]
patch
Comment 22 Saam Barati 2016-01-29 14:08:09 PST
Created attachment 270253 [details]
patch

rebased
Comment 23 WebKit Commit Bot 2016-01-29 14:10:18 PST
Attachment 270253 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:59:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:138:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:167:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:171:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:279:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Filip Pizlo 2016-01-29 14:17:56 PST
Comment on attachment 270253 [details]
patch

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

> Source/JavaScriptCore/b3/air/AirGenerate.cpp:239
> +    pcToOriginMap.appendItem(jit.label(), Origin());

Can you add a FIXME and file a bug about making late paths have an origin?
Comment 25 Saam Barati 2016-01-29 17:11:39 PST
landed in:
http://trac.webkit.org/changeset/195865