Bug 152729 - JSC Sampling Profiler: Detect tester and testee when sampling in RegExp JIT
Summary: JSC Sampling Profiler: Detect tester and testee when sampling in RegExp JIT
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 151713
Blocks: 182236
  Show dependency treegraph
 
Reported: 2016-01-04 23:06 PST by Saam Barati
Modified: 2019-09-11 15:41 PDT (History)
16 users (show)

See Also:


Attachments
Patch (30.88 KB, patch)
2018-01-02 09:06 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (31.84 KB, patch)
2018-01-02 10:50 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (35.95 KB, patch)
2018-01-03 00:35 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-01-04 23:06:26 PST
It will be sweet to know what regular expression we're executing and against what string.
Comment 1 Yusuke Suzuki 2018-01-02 09:06:51 PST
Created attachment 330322 [details]
Patch
Comment 2 Yusuke Suzuki 2018-01-02 09:08:41 PST
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'
Comment 3 EWS Watchlist 2018-01-02 09:59:40 PST
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.
Comment 4 Yusuke Suzuki 2018-01-02 10:50:47 PST
Created attachment 330324 [details]
Patch
Comment 5 EWS Watchlist 2018-01-02 10:52:30 PST
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.
Comment 6 Saam Barati 2018-01-02 11:23:02 PST
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.
Comment 7 Saam Barati 2018-01-02 11:25:22 PST
(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.
Comment 8 Yusuke Suzuki 2018-01-03 00:33:25 PST
(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.
Comment 9 Yusuke Suzuki 2018-01-03 00:35:39 PST
Created attachment 330369 [details]
Patch
Comment 10 EWS Watchlist 2018-01-03 00:37:25 PST
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.
Comment 11 Saam Barati 2018-01-18 17:03:09 PST
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?
Comment 12 Yusuke Suzuki 2018-01-18 17:08:46 PST
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.
Comment 13 Yusuke Suzuki 2018-01-25 20:06:33 PST
Ping?
Comment 14 Saam Barati 2018-01-28 23:28:33 PST
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?
Comment 15 Saam Barati 2018-01-28 23:28:56 PST
Comment on attachment 330369 [details]
Patch

r=me with inspector followups perhaps.
Comment 16 Yusuke Suzuki 2018-01-29 02:40:29 PST
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.
Comment 17 Yusuke Suzuki 2018-01-29 02:43:22 PST
Committed r227725: <https://trac.webkit.org/changeset/227725>
Comment 18 Radar WebKit Bug Importer 2018-01-29 02:44:21 PST
<rdar://problem/36980395>
Comment 19 Michael Catanzaro 2018-01-29 08:48:41 PST
This broke a bunch of JSC tests on Linux!
Comment 20 Matt Lewis 2018-01-29 09:47:34 PST
Reverted r227725 for reason:

This caused internal failures.

Committed r227738: <https://trac.webkit.org/changeset/227738>
Comment 22 Yusuke Suzuki 2019-09-10 19:12:16 PDT
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.
Comment 23 Yusuke Suzuki 2019-09-11 15:40:48 PDT
I'll open a new bug for introducing this feature in more reliable way.
Comment 24 Yusuke Suzuki 2019-09-11 15:41:51 PDT
https://bugs.webkit.org/show_bug.cgi?id=201702