Bug 156331 - ES6: Implement RegExp.prototype[@@search].
Summary: ES6: Implement RegExp.prototype[@@search].
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 156013 156557
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-06 22:19 PDT by Mark Lam
Modified: 2016-04-19 17:02 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-04-06 22:19:41 PDT
Patch coming.
Comment 1 Radar WebKit Bug Importer 2016-04-06 22:20:57 PDT
<rdar://problem/25594666>
Comment 2 Mark Lam 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.
Comment 3 Mark Lam 2016-04-12 16:51:07 PDT
Created attachment 276290 [details]
proposed patch.
Comment 4 Mark Lam 2016-04-12 16:54:20 PDT
Created attachment 276291 [details]
proposed patch.
Comment 5 Mark Lam 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.
Comment 6 Keith Miller 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.
Comment 7 Mark Lam 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.
Comment 8 Mark Lam 2016-04-13 12:31:51 PDT
Created attachment 276342 [details]
proposed patch: with IsRegExpObjectIntrinsic.
Comment 9 Mark Lam 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.
Comment 10 Keith Miller 2016-04-13 12:46:43 PDT
Comment on attachment 276342 [details]
proposed patch: with IsRegExpObjectIntrinsic.

r=me
Comment 11 Mark Lam 2016-04-13 13:01:15 PDT
Thanks for the reviews.  Landed in r199511: <http://trac.webkit.org/r199511>.
Comment 12 WebKit Commit Bot 2016-04-13 14:56:39 PDT
Re-opened since this is blocked by bug 156557
Comment 13 Mark Lam 2016-04-19 16:54:26 PDT
Created attachment 276776 [details]
in-browser sunspider baseline result on MBP
Comment 14 Mark Lam 2016-04-19 16:54:50 PDT
Created attachment 276777 [details]
in-browser sunspider new result on MBP
Comment 15 Mark Lam 2016-04-19 16:55:34 PDT
In browser sunspider runs show no regression.  Will re-land.
Comment 16 Mark Lam 2016-04-19 17:02:19 PDT
Re-landed in r199748: <http://trac.webkit.org/r199748>.