Bug 152629 – JSC Sampling Profiler: Come up with a (program counter => CodeOrigin) mapping

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

Created attachment 269310 [details]
WIP

it's a start.

[Saam Barati]

Created attachment 269522 [details]
WIP

[Saam Barati]

Created attachment 269532 [details]
WIP

now accounting for inlined frames at the top of the stack in a stack traces.

[Saam Barati]

Created attachment 269814 [details]
WIP

I think it's almost done.

[Saam Barati]

Created attachment 269839 [details]
more cleanup

[Saam Barati]

Created attachment 269843 [details]
more cleanup

[Saam Barati]

Created attachment 269911 [details]
make findPC find more PCs

[Saam Barati]

Created attachment 269913 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 269921 [details]
patch

hopefully fix builds.
fix some style issues

[Saam Barati]

Created attachment 269924 [details]
patch

rebased

[WebKit Commit Bot]

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.

[Saam Barati]

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().

[Saam Barati]

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

[Saam Barati]

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)

[Saam Barati]

Created attachment 269947 [details]
patch

[WebKit Commit Bot]

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.

[Keith Miller]

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.

[Saam Barati]

(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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 270246 [details]
patch

[Saam Barati]

Created attachment 270253 [details]
patch

rebased

[WebKit Commit Bot]

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.

[Filip Pizlo]

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?

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/195865