It will be sweet to know what regular expression we're executing and against what string.
Created attachment 330322 [details] Patch
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'
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.
Created attachment 330324 [details] Patch
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.
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.
(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.
(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.
Created attachment 330369 [details] Patch
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 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 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.
Ping?
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 on attachment 330369 [details] Patch r=me with inspector followups perhaps.
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.
Committed r227725: <https://trac.webkit.org/changeset/227725>
<rdar://problem/36980395>
This broke a bunch of JSC tests on Linux!
Reverted r227725 for reason: This caused internal failures. Committed r227738: <https://trac.webkit.org/changeset/227738>
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
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.
I'll open a new bug for introducing this feature in more reliable way.
https://bugs.webkit.org/show_bug.cgi?id=201702