Bug 202475 - [JSC] Implement RegExp Match Indices proposal
Summary: [JSC] Implement RegExp Match Indices proposal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL: https://github.com/tc39/proposal-rege...
Keywords: InRadar
Depends on: 204067
Blocks: 222278
  Show dependency treegraph
 
Reported: 2019-10-02 09:45 PDT by Ron Buckton
Modified: 2021-02-22 10:42 PST (History)
10 users (show)

See Also:


Attachments
Work in progress patch (18.04 KB, patch)
2020-10-06 16:24 PDT, Michael Saboff
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (63.66 KB, patch)
2020-11-11 18:24 PST, Michael Saboff
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Updated Patch addressing test failures (77.54 KB, patch)
2020-11-13 09:41 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch using 'd' flag conforming to latest proposal (39.04 KB, patch)
2021-01-25 10:15 PST, Michael Saboff
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Updated patch with 'd' flag and recommended fixes (42.57 KB, patch)
2021-02-17 14:21 PST, Michael Saboff
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Updated patch addressing review comments (46.94 KB, patch)
2021-02-18 10:24 PST, Michael Saboff
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ron Buckton 2019-10-02 09:45:05 PDT
Proposal Repository: https://github.com/tc39/proposal-regexp-match-indices

This proposal is currently at Stage 3 and is seeking implementations.
Comment 1 Radar WebKit Bug Importer 2020-09-30 10:29:10 PDT
<rdar://problem/69797861>
Comment 2 Michael Saboff 2020-10-06 16:24:48 PDT
Created attachment 410711 [details]
Work in progress patch
Comment 3 Michael Saboff 2020-10-06 16:25:43 PDT
Comment on attachment 410711 [details]
Work in progress patch

This implements a straightforward implementation.  Uploaded here for EWS testing.
Comment 4 Michael Saboff 2020-11-11 18:24:29 PST
Created attachment 413897 [details]
Patch
Comment 5 Michael Saboff 2020-11-13 09:41:53 PST
Created attachment 414055 [details]
Updated Patch addressing test failures
Comment 6 Yusuke Suzuki 2020-11-30 12:08:54 PST
Comment on attachment 414055 [details]
Updated Patch addressing test failures

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

> JSTests/stress/dfg-strength-reduction-for-regexp-should-indices-property.js:12
> +    // shouldBe(m.indices[0][1], 1);

Is this commented out intentionally?

> LayoutTests/ChangeLog:11
> +        * js/regexp-named-capture-groups-expected.txt:
> +        * js/script-tests/regexp-named-capture-groups.js:

Can we add these tests in JSTests/stress, and skip them for 32bit architectures? We will see failures in watchOS ARMv7k.
Comment 7 Michael Saboff 2020-11-30 13:26:24 PST
(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 414055 [details]
> Updated Patch addressing test failures
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414055&action=review
> 
> > JSTests/stress/dfg-strength-reduction-for-regexp-should-indices-property.js:12
> > +    // shouldBe(m.indices[0][1], 1);
> 
> Is this commented out intentionally?

The leading comment should be removed.  It was for patch testing.

> > LayoutTests/ChangeLog:11
> > +        * js/regexp-named-capture-groups-expected.txt:
> > +        * js/script-tests/regexp-named-capture-groups.js:
> 
> Can we add these tests in JSTests/stress, and skip them for 32bit
> architectures? We will see failures in watchOS ARMv7k.

Yeah, I can do that.
Comment 8 Yusuke Suzuki 2020-12-04 18:43:36 PST
Comment on attachment 414055 [details]
Updated Patch addressing test failures

Removing r? for now since the spec will be changed.
Comment 9 Yusuke Suzuki 2021-01-17 15:56:18 PST
https://github.com/tc39/proposal-regexp-match-indices/pull/49 Now, "d" flag is merged, so we need to update to integrate "d" flag.
Comment 10 Michael Saboff 2021-01-25 10:15:30 PST
Created attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal
Comment 11 Yusuke Suzuki 2021-01-25 12:04:35 PST
Comment on attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

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

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:106
> +            array->putDirect(vm, vm.propertyNames->indices, indicesArray);

I think this is causing GC and failure in mac-debug-wk1 bot.
Comment 12 Yusuke Suzuki 2021-01-25 12:44:30 PST
Comment on attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

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

>> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:106
>> +            array->putDirect(vm, vm.propertyNames->indices, indicesArray);
> 
> I think this is causing GC and failure in mac-debug-wk1 bot.

We need to set this property at the end of createRegExpMatchesArray (after ensuring GC is ok).
Comment 13 Yusuke Suzuki 2021-01-25 13:09:44 PST
Since RegExpMatchesArray's structure is different based on 'd' flag. This means that we also need to change DFG AbstractInterpreter's code.
Comment 14 Yusuke Suzuki 2021-01-25 13:10:11 PST
(In reply to Yusuke Suzuki from comment #13)
> Since RegExpMatchesArray's structure is different based on 'd' flag. This
> means that we also need to change DFG AbstractInterpreter's code.

  2558     case RegExpExec:
  2559     case RegExpExecNonGlobalOrSticky:
  2560         if (node->op() == RegExpExec) {
  2561             // Even if we've proven known input types as RegExpObject and String,
  2562             // accessing lastIndex is effectful if it's a global regexp.
  2563             clobberWorld();
  2564         }
  2565
  2566         if (JSValue globalObjectValue = forNode(node->child1()).m_value) {
  2567             if (JSGlobalObject* globalObject = jsDynamicCast<JSGlobalObject*>(m_vm, globalObjectValue)) {
  2568                 if (!globalObject->isHavingABadTime()) {
  2569                     m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
  2570                     RegisteredStructureSet structureSet;
  2571                     structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesArrayStructure()));
  2572                     setForNode(node, structureSet);
  2573                     forNode(node).merge(SpecOther);
  2574                     break;
  2575                 }
  2576             }
  2577         }
  2578         setTypeForNode(node, SpecOther | SpecArray);
  2579         break;

Whether we can use regExpMatchesArrayStructure() depends on "d" flag of RegExp.
Comment 15 Yusuke Suzuki 2021-01-28 01:34:24 PST
Comment on attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

Clearing the flag for now since we have several things which need to be solved.
Comment 16 Michael Saboff 2021-02-17 14:21:18 PST
Created attachment 420713 [details]
Updated patch with 'd' flag and recommended fixes
Comment 17 Yusuke Suzuki 2021-02-17 16:51:07 PST
Comment on attachment 420713 [details]
Updated patch with 'd' flag and recommended fixes

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2628
> +                    // If regExp is unknown, we need to put both regExp MatchesArray structure variants in our set.
> +                    if (!regExp || !regExp->hasIndices())
> +                        structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesArrayStructure()));
> +                    if (!regExp || regExp->hasIndices())
> +                        structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesIndicesArrayStructure()));

`regExpMatchesIndicesArrayStructure` is structure for array of indices property. This is not the result array's structure.
We need to add `regExpMatchesArrayWithIndicesStructure`, and need to pre-bake the structure which includes indices property, and use this structure when creating the result array.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:35
>  static const PropertyOffset RegExpMatchesArrayGroupsPropertyOffset = 102;

We should add PropertyOffset for RegExpMatchesArrayIndicesPropertyOffset. And we should pre-bake structure for that RegExpMatchesArray.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:89
>      Structure* matchStructure = globalObject->regExpMatchesArrayStructure();

matchStructure should be different if indices property exist.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:221
> +    if (createIndices)
> +        array->putDirect(vm, vm.propertyNames->indices, indicesArray);

We should set this inside setProperties by using RegExpMatchesArrayIndicesPropertyOffset.
Comment 18 Michael Saboff 2021-02-18 10:24:07 PST
Created attachment 420842 [details]
Updated patch addressing review comments
Comment 19 Yusuke Suzuki 2021-02-18 10:45:39 PST
Comment on attachment 420842 [details]
Updated patch addressing review comments

r=me
Comment 20 Michael Saboff 2021-02-18 11:16:27 PST
Committed r273086 (234284@main): <https://commits.webkit.org/234284@main>