Bug 156331 – ES6: Implement RegExp.prototype[@@search].

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/25594666>

[Mark Lam]

Created attachment 276045 [details]
proposed patch.

This patch should be functionally correct.  Remaining issues:

1. need to run tests and possibly rebase results (due to error messages being updated).

2. waiting for https://bugs.webkit.org/show_bug.cgi?id=156013 to land first because that patch introduces many pieces of code that this patch builds on.  In this patch, I copied those bits over.  Will have to rebase this patch after 156013 lands.

3. need to fix perf regressions.  Will wait for https://bugs.webkit.org/show_bug.cgi?id=156378 to land before re-testing for perf, and see if I need to add some additional optimizations to fix regressions.

[Mark Lam]

Created attachment 276290 [details]
proposed patch.

[Mark Lam]

Created attachment 276291 [details]
proposed patch.

[Mark Lam]

Created attachment 276296 [details]
x86_64 benchmark results.

Sadly, there's a perf regression:
   v8-regexp-search                                  68.5345+-2.8754     !     74.9810+-3.4322        ! definitely 1.0941x slower

I'll look into it.

[Keith Miller]

Comment on attachment 276291 [details]
proposed patch.

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

r=me with comments.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:119
> +        throw new @TypeError("RegExp.prototype.@@search requires that |this| be an Object");

I think it would be clearer to say "RegExp.prototype[Symbol.search] requires that |this| be an Object" but I don't know what our convention is.

[Mark Lam]

Thanks.

(In reply to comment #6)
> > Source/JavaScriptCore/builtins/RegExpPrototype.js:119
> > +        throw new @TypeError("RegExp.prototype.@@search requires that |this| be an Object");
> 
> I think it would be clearer to say "RegExp.prototype[Symbol.search] requires
> that |this| be an Object" but I don't know what our convention is.

I'm just keeping with the existing convention.  Let me do a subsequent refactoring patch to change all the error messages at once.  I'll keep it as is for now.

I still need to look into the perf regression for this patch first before landing.

[Mark Lam]

Created attachment 276342 [details]
proposed patch: with IsRegExpObjectIntrinsic.

[Mark Lam]

Created attachment 276343 [details]
x86_64 benchmark results.

Perf is neutral.  The "definitely"s in the benchmark results are all due to noise.

This patch has passed JSC and layout tests.

[Keith Miller]

Comment on attachment 276342 [details]
proposed patch: with IsRegExpObjectIntrinsic.

r=me

[Mark Lam]

Thanks for the reviews.  Landed in r199511: <http://trac.webkit.org/r199511>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156557

[Mark Lam]

Created attachment 276776 [details]
in-browser sunspider baseline result on MBP

[Mark Lam]

Created attachment 276777 [details]
in-browser sunspider new result on MBP

[Mark Lam]

In browser sunspider runs show no regression.  Will re-land.

[Mark Lam]

Re-landed in r199748: <http://trac.webkit.org/r199748>.