Bug 180988 – [DFG][FTL] regExpMatchFast should be handled

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[DFG][FTL] regExpMatchFast should be handled

[Yusuke Suzuki]

Created attachment 329795 [details]
Patch

[Keith Miller]

Looks like you upset the EWS. :(

[EWS Watchlist]

Comment on attachment 329795 [details]
Patch

Attachment 329795 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5757662

New failing tests:
css3/shapes/shape-outside/values/shape-outside-ellipse-004.html
css3/shapes/shape-outside/values/shape-outside-circle-004.html
imported/w3c/web-platform-tests/css/css-shapes-1/shape-outside/values/shape-outside-ellipse-004.html

[EWS Watchlist]

Created attachment 329808 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329795 [details]
Patch

Attachment 329795 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/5757696

New failing tests:
mozilla-tests.yaml/ecma_2/String/match-001.js.mozilla-dfg-eager-no-cjit-validate-phases
stress/regress-174044.js.ftl-no-cjit-small-pool
stress/regress-174044.js.default
stress/regress-174044.js.ftl-eager-no-cjit-b3o1
mozilla-tests.yaml/ecma_2/String/match-003.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma_2/String/match-001.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma_2/String/match-003.js.mozilla-dfg-eager-no-cjit-validate-phases
stress/regress-174044.js.dfg-eager-no-cjit-validate
stress/regress-174044.js.ftl-no-cjit-no-put-stack-validate
stress/regress-174044.js.dfg-eager
stress/regress-174044.js.no-cjit-collect-continuously
stress/regress-174044.js.no-ftl
stress/regress-174044.js.ftl-no-cjit-b3o1
stress/regress-174044.js.no-llint
stress/regress-174044.js.ftl-eager-no-cjit
stress/regress-174044.js.ftl-no-cjit-validate-sampling-profiler
stress/regress-174044.js.ftl-eager
stress/regress-174044.js.no-cjit-validate-phases
stress/regress-174044.js.dfg-maximal-flush-validate-no-cjit

[EWS Watchlist]

Comment on attachment 329795 [details]
Patch

Attachment 329795 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5757733

New failing tests:
css3/shapes/shape-outside/values/shape-outside-ellipse-004.html
css3/shapes/shape-outside/values/shape-outside-circle-004.html
svg/custom/object-sizing-explicit-width.xhtml
imported/w3c/web-platform-tests/css/css-shapes-1/shape-outside/values/shape-outside-ellipse-004.html

[EWS Watchlist]

Created attachment 329819 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329795 [details]
Patch

Attachment 329795 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5758585

New failing tests:
css3/shapes/shape-outside/values/shape-outside-ellipse-004.html
css3/shapes/shape-outside/values/shape-outside-circle-004.html
imported/w3c/web-platform-tests/css/css-shapes-1/shape-outside/values/shape-outside-ellipse-004.html

[EWS Watchlist]

Created attachment 329828 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 329986 [details]
Patch

[Yusuke Suzuki]

Created attachment 329987 [details]
Patch

[Yusuke Suzuki]

Created attachment 329988 [details]
Patch

[Yusuke Suzuki]

Ping?

[Joseph Pecoraro]

Comment on attachment 329988 [details]
Patch

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

Neat!

> Source/JavaScriptCore/builtins/RegExpPrototype.js:85
> +    'use strict';

Style: Most of the time this is a double quoted string.

[Yusuke Suzuki]

Comment on attachment 329988 [details]
Patch

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

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:85
>> +    'use strict';
> 
> Style: Most of the time this is a double quoted string.

Thanks, fixed.

[Yusuke Suzuki]

Created attachment 330509 [details]
Patch

[Yusuke Suzuki]

Created attachment 330510 [details]
Patch

[EWS Watchlist]

Comment on attachment 330510 [details]
Patch

Attachment 330510 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5934791

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[EWS Watchlist]

Created attachment 330516 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

(In reply to Build Bot from comment #19)
> Created attachment 330516 [details]
> Archive of layout-test-results from ews106 for mac-elcapitan-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

Seems unrelated.

[Yusuke Suzuki]

Ping?

[Mark Lam]

Comment on attachment 330510 [details]
Patch

r=me

[Mark Lam]

(In reply to Yusuke Suzuki from comment #20)
> (In reply to Build Bot from comment #19)
> > Created attachment 330516 [details]
> > Archive of layout-test-results from ews106 for mac-elcapitan-wk2
> > 
> > The attached test failures were seen while running run-webkit-tests on the
> > mac-wk2-ews.
> > Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
> 
> Seems unrelated.

Can you please run the failed test locally and confirm that it passes with your patch before landing?

[Saam Barati]

Comment on attachment 330510 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1968
> +    case RegExpMatch:

This name confused me. I vote for calling this RegExpMatchFast to align w/ the intrinsic. I originally thought this was the entire match operation, but it's only the fast path.

[Saam Barati]

Comment on attachment 330510 [details]
Patch

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

LGTM too

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2621
> +        if (argumentCountIncludingThis != 2)
> +            return false;

Seems like this should be a release assert since this is only called by our builtins.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:1026
> +EncodedJSValue JIT_OPERATION operationRegExpMatchString(ExecState* exec, JSGlobalObject* globalObject, RegExpObject* regExpObject, JSString* argument)

I would also add "fast" here to align the names.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:-149
> -    VM& vm = exec->vm();
> -    auto scope = DECLARE_THROW_SCOPE(vm);
> -
> -    JSValue thisValue = exec->thisValue();
> -    if (!thisValue.inherits(vm, RegExpObject::info()))
> -        return throwVMTypeError(exec, scope);
> -    JSString* string = exec->argument(0).toStringOrNull(exec);
> -    EXCEPTION_ASSERT(!!scope.exception() == !string);
> -    if (!string)
> -        return encodedJSValue();
> -    if (!asRegExpObject(thisValue)->regExp()->global()) {
> -        scope.release();
> -        return JSValue::encode(asRegExpObject(thisValue)->exec(exec, exec->lexicalGlobalObject(), string));
> -    }
> -    scope.release();
> -    return JSValue::encode(asRegExpObject(thisValue)->matchGlobal(exec, exec->lexicalGlobalObject(), string));

This was all unneeded because we already do the safety checks in the builtin, right? Why was this here before then?

[Yusuke Suzuki]

Comment on attachment 330510 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1968
>> +    case RegExpMatch:
> 
> This name confused me. I vote for calling this RegExpMatchFast to align w/ the intrinsic. I originally thought this was the entire match operation, but it's only the fast path.

OK, changed it to RegExpMatchFast.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2621
>> +            return false;
> 
> Seems like this should be a release assert since this is only called by our builtins.

Right. Changed.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:1026
>> +EncodedJSValue JIT_OPERATION operationRegExpMatchString(ExecState* exec, JSGlobalObject* globalObject, RegExpObject* regExpObject, JSString* argument)
> 
> I would also add "fast" here to align the names.

Fixed.

>> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:-149
>> -    return JSValue::encode(asRegExpObject(thisValue)->matchGlobal(exec, exec->lexicalGlobalObject(), string));
> 
> This was all unneeded because we already do the safety checks in the builtin, right? Why was this here before then?

Right. This function is only called if the above check is met. So this is unnecessary. I'm not sure why we have these unnecessary checks.

[Yusuke Suzuki]

(In reply to Mark Lam from comment #23)
> (In reply to Yusuke Suzuki from comment #20)
> > (In reply to Build Bot from comment #19)
> > > Created attachment 330516 [details]
> > > Archive of layout-test-results from ews106 for mac-elcapitan-wk2
> > > 
> > > The attached test failures were seen while running run-webkit-tests on the
> > > mac-wk2-ews.
> > > Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
> > 
> > Seems unrelated.
> 
> Can you please run the failed test locally and confirm that it passes with
> your patch before landing?

OK, the test runs as expected in my working copy.

[Yusuke Suzuki]

Committed r226775: <https://trac.webkit.org/changeset/226775>

[Radar WebKit Bug Importer]

<rdar://problem/36436985>