Bug 152729 – JSC Sampling Profiler: Detect tester and testee when sampling in RegExp JIT

• Status: RESOLVED
• Resolution: LATER
• Component: JavaScriptCore

[Saam Barati]

It will be sweet to know what regular expression we're executing and against what string.

[Yusuke Suzuki]

Created attachment 330322 [details]
Patch

[Yusuke Suzuki]

When executing Octane/regexp with Octane's test runner, we get the following output. We can see that "/\s?;\s?/" takes much time.

Sampling rate: 1000.000000 microseconds
Top functions as <numSamples  'functionName:sourceID'>
   773    'Exec:2'
   580    'runBlock1:2'
   210    'replace:3'
    89    'runBlock0:2'
    58    '/\s?;\s?/:-1'
    45    ':-1'
    20    'runBlock6:2'
    17    'runBlock11:2'
    13    'runBlock5:2'
     9    'runBlock2:2'
     9    'runBlock3:2'
     8    'runBlock9:2'


Sampling rate: 1000.000000 microseconds
Hottest bytecodes as <numSamples   'functionName#hash:JITType:bytecodeIndex'>
   192    'replace#C4Vrxg:FTL:105'
   134    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:235'
   127    'Exec#B1957w:FTL:34'
    81    'runBlock1#AJeMRS:FTL:1860'
    79    'runBlock1#AJeMRS:FTL:1795'
    74    'runBlock1#AJeMRS:DFG:1860'
    64    'runBlock1#AJeMRS:FTL:1925'
    59    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:565'
    58    '/\s?;\s?/#<nil>:None:<nil>'
    56    'runBlock1#AJeMRS:DFG:1795'
    56    'runBlock1#AJeMRS:DFG:1925'
    45    '#<nil>:None:<nil>'
    43    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:727'
    36    'runBlock1#AJeMRS:FTL:271'
    35    'runBlock1#AJeMRS:FTL:348'
    33    'runBlock1#AJeMRS:DFG:348'
    25    'runBlock0#BKLcUC:FTL:394'
    23    'runBlock1#AJeMRS:DFG:271'
    21    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:164'
    20    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:DFG:235'
    15    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:DFG:565'
    14    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:2354'
    12    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:1201'
    11    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:1841'
    11    'replace#C4Vrxg:DFG:105'
    10    'Exec#B1957w:Baseline:34 <-- runBlock1#AJeMRS:FTL:2289'
    10    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:1039'
     9    'Exec#B1957w:Baseline:34 <-- runBlock3#BdH8wB:DFG:2100'
     9    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:1608'
     8    'runBlock0#BKLcUC:FTL:3368'
     8    'Exec#B1957w:DFG:34'
     7    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:877'
     7    'runBlock1#AJeMRS:FTL:1990'
     7    'runBlock0#BKLcUC:DFG:394'
     7    'anonymous#<nil>:None:<nil>'
     7    'Exec#B1957w:Baseline:34 <-- runBlock1#AJeMRS:FTL:1055'
     7    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:326'
     6    'Exec#B1957w:Baseline:34 <-- runBlock1#AJeMRS:DFG:2289'
     6    '/[+, ]/#<nil>:None:<nil>'
     6    'Exec#B1957w:Baseline:34 <-- runBlock0#BKLcUC:FTL:2242'

[EWS Watchlist]

Attachment 330322 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:121:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:229:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 330324 [details]
Patch

[EWS Watchlist]

Attachment 330324 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:121:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:229:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Before landing this we should make sure that when the concurrent JIT runs in the YARR, we don't think the main thread is running the RegExp JIT. This bug exists in ToT today. This probably doesn't have any real impact today, but with this change, it might, since it may be racy.

[Saam Barati]

(In reply to Saam Barati from comment #6)
> Before landing this we should make sure that when the concurrent JIT runs in
> the YARR, we don't think the main thread is running the RegExp JIT. This bug
> exists in ToT today. This probably doesn't have any real impact today, but
> with this change, it might, since it may be racy.

I'm saying this before reading the code. I'll check out the code soon.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #6)
> Before landing this we should make sure that when the concurrent JIT runs in
> the YARR, we don't think the main thread is running the RegExp JIT. This bug
> exists in ToT today. This probably doesn't have any real impact today, but
> with this change, it might, since it may be racy.

Interesting. I updated the patch to handle it correctly. I'll upload the patch soon.

[Yusuke Suzuki]

Created attachment 330369 [details]
Patch

[EWS Watchlist]

Attachment 330369 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/RegExp.h:76:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:121:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:229:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 330369 [details]
Patch

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

> JSTests/stress/sampling-profiler-regexp.js:17
> +    runTest(baz, ["/.*/gi", "", "test", "baz"]);

This test doesn't fail when we turn off YARR at runtime?

[Yusuke Suzuki]

Comment on attachment 330369 [details]
Patch

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

>> JSTests/stress/sampling-profiler-regexp.js:17
>> +    runTest(baz, ["/.*/gi", "", "test", "baz"]);
> 
> This test doesn't fail when we turn off YARR at runtime?

I think this test fails if we set `useRegExpJIT = false`. But i think this does not fail at buildbots since JIT-disabled bots (CLoop) do not execute this test since sampling profiler is not available.

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 330369 [details]
Patch

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

LGTM, but my main worry is what Web Inspector shows for RegExp frames. Can you share a screenshot of what that looks like? Maybe we need to make some enhancements there in a follow up.

> Source/JavaScriptCore/runtime/RegExp.h:76
> +    enum class ExecutionContext { JS, NonJS };

This name doesn't feel 100% accurate to me. Technically, the compiler is still worried about JS. I think we should name this something along the lines of MainThread vs BackgroundThread. (Maybe we already have an enum that describes that?)

> Source/JavaScriptCore/runtime/RegExp.h:171
> +            if (m_vm)
> +                m_vm->executingRegExpJIT = nullptr;

Since you directly set this to nullptr, you should assert in the constructor that it's nullptr (e.g, verify that it's not re-entrant).

> Source/JavaScriptCore/runtime/VM.h:599
> +    RegExp* executingRegExpJIT { nullptr };

Maybe: currentlyExecutingRegExp?

[Saam Barati]

Comment on attachment 330369 [details]
Patch

r=me with inspector followups perhaps.

[Yusuke Suzuki]

Comment on attachment 330369 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/RegExp.h:76
>> +    enum class ExecutionContext { JS, NonJS };
> 
> This name doesn't feel 100% accurate to me. Technically, the compiler is still worried about JS. I think we should name this something along the lines of MainThread vs BackgroundThread. (Maybe we already have an enum that describes that?)

Yeah, we have this. enum class Concurrency in ObjectPropertyConditionSet.cpp.

>> Source/JavaScriptCore/runtime/RegExp.h:171
>> +                m_vm->executingRegExpJIT = nullptr;
> 
> Since you directly set this to nullptr, you should assert in the constructor that it's nullptr (e.g, verify that it's not re-entrant).

Nice. Fixed.

>> Source/JavaScriptCore/runtime/VM.h:599
>> +    RegExp* executingRegExpJIT { nullptr };
> 
> Maybe: currentlyExecutingRegExp?

Sounds nice. Fixed.

[Yusuke Suzuki]

Committed r227725: <https://trac.webkit.org/changeset/227725>

[Radar WebKit Bug Importer]

<rdar://problem/36980395>

[Michael Catanzaro]

This broke a bunch of JSC tests on Linux!

[Matt Lewis]

Reverted r227725 for reason:

This caused internal failures.

Committed r227738: <https://trac.webkit.org/changeset/227738>

[Matt Lewis]

This also cause JSC test on High Sierra as well as the CLoop Tests.

JSC:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20JSC%20%28Tests%29/builds/2021

CLoop:
https://build.webkit.org/builders/Apple%20High%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3396

[Yusuke Suzuki]

I think we should rebaseline this patch with more reliable manner.

1. RegExp JSCell will be stored in CallFrame like WasmCallee / CodeBlock
2. SamplingProfiler will detect RegExp JSCell.

I really want to have this SamplingProfiler feature to examine what RegExp is taking time in JetStream2 benchmarks, like, JetStream2/OfflineAssembler.

[Yusuke Suzuki]

I'll open a new bug for introducing this feature in more reliable way.

[Yusuke Suzuki]

https://bugs.webkit.org/show_bug.cgi?id=201702