Bug 156331

Summary:

ES6: Implement RegExp.prototype[@@search].

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, commit-queue, fpizlo, ggaren, keith_miller, msaboff, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

156013, 156557

Bug Blocks:

Attachments:

Description	Flags
proposed patch.	none
proposed patch.	none
proposed patch.	keith_miller: review+
x86_64 benchmark results.	none
proposed patch: with IsRegExpObjectIntrinsic.	keith_miller: review+
x86_64 benchmark results.	none
in-browser sunspider baseline result on MBP	none
in-browser sunspider new result on MBP	none

Mark Lam

Reported 2016-04-06 22:19:41 PDT

Patch coming.

Attachments
proposed patch. (43.29 KB, patch) 2016-04-08 15:27 PDT, Mark Lam	no flags	Details Formatted Diff Diff
proposed patch. (45.16 KB, patch) 2016-04-12 16:51 PDT, Mark Lam	no flags	Details Formatted Diff Diff
proposed patch. (45.23 KB, patch) 2016-04-12 16:54 PDT, Mark Lam	keith_miller: review+	Details Formatted Diff Diff
x86_64 benchmark results. (71.30 KB, text/plain) 2016-04-12 17:33 PDT, Mark Lam	no flags	Details
proposed patch: with IsRegExpObjectIntrinsic. (62.77 KB, patch) 2016-04-13 12:31 PDT, Mark Lam	keith_miller: review+	Details Formatted Diff Diff
x86_64 benchmark results. (71.41 KB, text/plain) 2016-04-13 12:39 PDT, Mark Lam	no flags	Details
in-browser sunspider baseline result on MBP (1.60 KB, text/plain) 2016-04-19 16:54 PDT, Mark Lam	no flags	Details
in-browser sunspider new result on MBP (1.61 KB, text/plain) 2016-04-19 16:54 PDT, Mark Lam	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2016-04-06 22:20:57 PDT

<rdar://problem/25594666>

Mark Lam

Comment 2 2016-04-08 15:27:56 PDT

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

Comment 3 2016-04-12 16:51:07 PDT

Created attachment 276290 [details] proposed patch.

Mark Lam

Comment 4 2016-04-12 16:54:20 PDT

Created attachment 276291 [details] proposed patch.

Mark Lam

Comment 5 2016-04-12 17:33:03 PDT

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 6 2016-04-12 17:48:13 PDT

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

Comment 7 2016-04-12 17:52:57 PDT

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

Comment 8 2016-04-13 12:31:51 PDT

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

Mark Lam

Comment 9 2016-04-13 12:39:59 PDT

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 10 2016-04-13 12:46:43 PDT

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

Mark Lam

Comment 11 2016-04-13 13:01:15 PDT

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

WebKit Commit Bot

Comment 12 2016-04-13 14:56:39 PDT

Re-opened since this is blocked by bug 156557

Mark Lam

Comment 13 2016-04-19 16:54:26 PDT

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

Mark Lam

Comment 14 2016-04-19 16:54:50 PDT

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

Mark Lam

Comment 15 2016-04-19 16:55:34 PDT

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

Mark Lam

Comment 16 2016-04-19 17:02:19 PDT

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

Note You need to log in before you can comment on or make changes to this bug.