Implement ES6 RegExp.prototype methods and change String.prototype methods to delegate

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: bbenvie, Assigned: arai)

Tracking

({dev-doc-complete})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(34 attachments, 67 obsolete attachments)

120.63 KB, image/png
Details
15.59 KB, image/png
Details
25.36 KB, image/png
Details
5.00 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.27 KB, patch
arai
: review+
Details | Diff | Splinter Review
6.33 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.30 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.91 KB, patch
arai
: review+
Details | Diff | Splinter Review
27.41 KB, patch
arai
: review+
Details | Diff | Splinter Review
24.83 KB, patch
arai
: review+
Details | Diff | Splinter Review
10.89 KB, patch
arai
: review+
Details | Diff | Splinter Review
44.26 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.54 KB, patch
till
: review+
Details | Diff | Splinter Review
23.06 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.01 KB, patch
till
: review+
Details | Diff | Splinter Review
4.03 KB, patch
arai
: review+
Details | Diff | Splinter Review
37.33 KB, patch
till
: review+
Details | Diff | Splinter Review
92.63 KB, patch
till
: review+
Details | Diff | Splinter Review
1.38 KB, patch
till
: review+
Details | Diff | Splinter Review
4.27 KB, patch
arai
: review+
Details | Diff | Splinter Review
4.00 KB, patch
arai
: review+
Details | Diff | Splinter Review
17.55 KB, patch
till
: review+
Details | Diff | Splinter Review
60.21 KB, patch
till
: review+
Details | Diff | Splinter Review
43.94 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.88 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.88 KB, patch
arai
: review+
Details | Diff | Splinter Review
950 bytes, patch
arai
: review+
Details | Diff | Splinter Review
8.69 KB, patch
till
: review+
Details | Diff | Splinter Review
1.74 KB, patch
till
: review+
Details | Diff | Splinter Review
1.72 KB, patch
till
: review+
Details | Diff | Splinter Review
1.33 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.93 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.00 KB, patch
till
: review+
Details | Diff | Splinter Review
1.00 KB, patch
till
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
A number of methods have been added to `RegExp.prototype` that correspond to existing `String.prototype` methods which can accept RegExp's.

ES6 spec (May 2013 draft) sections:

* RegExp.prototype.match(string) - 15.10.6.11
* RegExp.prototype.replace(S, replaceValue) - 15.10.6.12
* RegExp.prototype.search(S) - 15.10.6.13
* RegExp.prototype.split(string, limit) - 15.10.6.14

Furthermore, the corresponding `String.prototype` methods of the same names should delegate to these methods when a RegExp is given as the matcher. For example:

> let regexp = /\s/
> let str = 'a b c d'
> str.split(regex)

Should result in the invocation `regexp.split(str)`. This is observable to users if they modify the methods on `RegExp.prototype` or if they add correctly named methods to RegeExp instances.

> let regexp = /./
> regexp.split = () => 'could be anything'
> ''.split(regexp) // 'could be anything'


See also: https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-01/jan-29.md (search for "RegExp")
Assignee: general → nobody
These have all been changed to symbols instead of strings:
https://twitter.com/awbjs/status/535854844787957762

Updated

4 years ago
Blocks: 1119779
No longer blocks: es6
(Assignee)

Comment 2

4 years ago
depends on bug 1108382, since String.prototype.{match,search,replace} has non-standard flags argument now, and it shouldn't be a good idea to introduce it to RegExp.prototype[{@@match,@@search,@@replace}] which is customizable.
Depends on: 1108382
Whiteboard: [DocArea=JS]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1163757
(Assignee)

Comment 4

4 years ago
Looks like flags argument can be handled in String methods, not RegExp methods.  So I now don't think bug 1108382 blocks this.
Basic implementation is almost done, now tweaking performance.  Performance regression is expected, anyways, because we can't optimize for string case in match/search, and we should explicitly call exec method.
No longer depends on: 1108382
(Assignee)

Updated

4 years ago
Duplicate of this bug: 817353
(Assignee)

Updated

4 years ago
Duplicate of this bug: 817351
(Assignee)

Updated

4 years ago
Duplicate of this bug: 877880
(Assignee)

Comment 9

4 years ago
Thanks!  I'll implement with ToUint32, with some note about that :)
(Assignee)

Comment 10

4 years ago
In R.p[@@split], sticky flag in R.p.exec needs to conform to spec before implementing this.

expected
  let r = /\b/y;
  r.lastIndex = 0;
  r.exec("abc")    // [""]
  r.lastIndex = 1;
  r.exec("abc")    // null
  r.lastIndex = 2;
  r.exec("abc")    // null
  r.lastIndex = 3;
  r.exec("abc")    // [""]

actual
  let r = /\b/y;
  r.lastIndex = 0;
  r.exec("abc")    // [""]
  r.lastIndex = 1;
  r.exec("abc")    // [""]
  r.lastIndex = 2;
  r.exec("abc")    // [""]
  r.lastIndex = 3;
  r.exec("abc")    // null
(Assignee)

Comment 11

4 years ago
It's also true for ^, in bug 773687.
Depends on: 773687
(Assignee)

Comment 12

4 years ago
Implemented RegExp.prototype[{@@match,@@search,@@replace,@@split}] with Self-hosted JS, and also changed String.prototype methods to call them, also with Self-hosted JS.
String.prototype.replace and String.prototype.split still uses jitted code if argument is string, internally.

flags argument is still supported, in String.prototype methods' side.  String.prototype.replace is somehow tricky, since it doesn't recognize RegExp pattern, but ignoreCase is supported.  Currently it calls RegExp.prototype[@@replace] with escaped pattern.

Here's try run (and patches), for bug 773687 (sticky), bug 1135377 (unicode), and this bug.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0427150eeb49

Passed almost tests, but some tests hit timeout, caused by performance regression (see attached image).
Parser test can be fixed by just removing String.prototype.replace calls there, since it's not a test for replace. (I'll file it shortly)
Not yet checked others, but, current performance won't be acceptable.

If RegExp.prototype.exec can become faster, the performance regression could be reduced.
Assignee: nobody → arai.unmht
Very nice evaluation!

Needinfo'ing h4writer because he's done some RegExp perf work and might be able to help here.
Flags: needinfo?(hv1989)
Sorry about the delay. I'll definitely take a look tomorrow, maybe this afternoon.
(Assignee)

Comment 15

4 years ago
thank you :)


Forgot to note about RegExpStatics in the patches.

String.prototype.replace saves/restores RegExpStatics before/after calling replaceValue function (no spec, and different behaviors between engines! *1).  To implement with Self-hosted JS, it needs native function or try-catch to restore when replaceValue throws.  In current patch I use native function, but this won't be good for performance.

Also, with ES6's replace method, RegExp.prototype.exec may not be called (if .exec is modified or overridden), and it means there may be nothing to save/restore in RegExpStatics.  I'm wondering how to handle it.  In current patch I save and restore only if there's RegExpStatics value (so, if .exec is called before), but we don't know if it's the result related to replace operation, and we may restore wrong(?) value when leaving @@replace.

--

*1 V8 and JSC updates RegExp.$1 value before calling replaceValue,
   Chakra updates RegExp.$1 value after calling replaceValue only if replaceValue didn't throw,
   and, SpiderMonkey updates RegExp.$1 before calling replaceValue, and restores the value after calling replaceValue, even if it threw,
   If we can switch to V8 and JSC's behavior, it gets so simpler, but it will break something...
(In reply to Tooru Fujisawa [:arai] from comment #15)
> thank you :)
> 
> 
> Forgot to note about RegExpStatics in the patches.
> 
> String.prototype.replace saves/restores RegExpStatics before/after calling
> replaceValue function (no spec, and different behaviors between engines!
> *1).  To implement with Self-hosted JS, it needs native function or
> try-catch to restore when replaceValue throws.  In current patch I use
> native function, but this won't be good for performance.

Using try/catch should be ok: we've had zero-overhead try blocks for quite a while. But ...

>    If we can switch to V8 and JSC's behavior, it gets so simpler, but it
> will break something...

I think we should at least try this. If it works, we not only improve our implementation but also decrease fragmentation of the web platform's behavior.
(Assignee)

Comment 17

4 years ago
(In reply to Till Schneidereit [:till] from comment #16)
> Using try/catch should be ok: we've had zero-overhead try blocks for quite a
> while.

wow, that's nice :)

> >    If we can switch to V8 and JSC's behavior, it gets so simpler, but it
> > will break something...
> 
> I think we should at least try this. If it works, we not only improve our
> implementation but also decrease fragmentation of the web platform's
> behavior.

pushed to try, with commenting out PreserveRegExpStatics:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bf37ee477d0
(Assignee)

Updated

4 years ago
See Also: → 1207922
1) String.prototype.match(regexp/g): (discussed on irc) The issue here is that we used to do
> js code
>          -> c++ str_match
>                        -> irregexp jit
>                        -> irregexp jit 
>                        -> ... 
with this patch we do 
> js code
>          -> js "match"
>                        -> c++ regexp_exec_raw
>                                -> irregexp jit 
>                        -> c++ regexp_exec_raw
>                                -> irregexp jit 
>                        -> ... 
since we cannot go from LRegExpExec to irregexp_jit immediately (bug 1187438). We take a detour since we cannot set lastIndex. The proposal here is to make LRegExpMatch do only a subset of LRegExpExec (ES6 21.2.5.2.2 step 6 and higher) and selfhost the rest (step 1-5). That way we can also do global matching.

2) String.prototype.match(string)
We have a fastpath for https://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#2426. That eliminates the need to run "RegExpObject". Which is off course slower.

3) I think the other things are mostly some small optimizations. I see some NativeCalls that should have been eliminated E.g. ToObject intrinsic that isn't replaced in MCallOptimize into MToObject. But I think the above two issues are the biggest issues currently.
Flags: needinfo?(hv1989)
4) var matcher = GetMethod(regexp, std_match); is quite expensive for strings.

Removing this gives me "1.2s" instead of "1.6s". A 33% improvement. This code wasn't present in the old ES5 code. So part of the reason of the slowdown. Also this is sub-optimally transformed into jit code. The ToObject this does has a string as input, which we don't support and use C++ code. Implementing that should definitely increase score a bit.
We could also think outside the box. I think matcher will only be non-undefined when Object.prototype[@@match] and String.prototype[@@match] isn't overridden. Would it be easier to test this for string inputs?
(In reply to Hannes Verschore [:h4writer] from comment #19)
> The ToObject this does has a string as input, which we don't support and use C++ code.

Removing the (invalid!) ToObject call in GetMethod is tracked in bug 1204562.
(Assignee)

Comment 21

4 years ago
About selfhosting R.p.exec (and R.p.test too), I think we can split out all lastIndex-related steps to selfhosted, as 21.2.5.2.2 steps 19-29 are not observable (am I correct?) and we can reorder step 18 after those steps.  The value of |e| in Step 18.a can be re-calculated with |A[0].index + A[0].length| for R.p.exec's case, as those steps are also not observable (would be better if we can get the value directly tho), and it could be possible to return |e| value itself from JIT code for R.p.test's case (using -1 for false) instead of true/false.
(Assignee)

Updated

4 years ago
See Also: → 1208819
(Assignee)

Updated

4 years ago
See Also: → 1208835
(Assignee)

Updated

4 years ago
Depends on: 1208819
(Assignee)

Comment 22

4 years ago
Applied 3 changes

1. self-hosted RegExp.prototype.exec and test

match/search/replace with regexp/g, and split get faster from previous one.
non-loop tests and interpreter execution gets slower of course.

2. removed PreserveRegExpStatics from self-hosted version of replace

Ion execution per each iteration is now faster than original.

3. add check for Object/String.prototype[@@match] etc to each String method

about 5% performance improvement on string case.


Here's try run and patches for this bug, bug 1207922, bug 1135377, bug 1208819, and RegExp[@@species].
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30312d07b2fa

still hitting some TIMEOUT on cgc.
Attachment #8663403 - Attachment is obsolete: true

Updated

4 years ago
Duplicate of this bug: 1225009
(Assignee)

Comment 24

4 years ago
Posted file WIP patches (obsolete) —
(Assignee)

Comment 25

4 years ago
Improved String.prototype.search(string) case
Attachment #8672540 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Posted image Performance comparison of exec/test (obsolete) —
(Assignee)

Comment 27

4 years ago
Current main performance issue is String.prototype.match(string) and String.prototype.search(string)

especially String.prototype.search(string) is extremely faster on current m-c

following will improve it

* avoid checking @@match/@@search property of regexp argument at all
currently, if regexp argument is a string, StringHasNoMatch(Part 13) and StringHasNoSearch(Part 16) check the @@match/@@search property of Object.prototype and String.prototype.  If we can optimize that away in JIT, the performance will be improved.

* inline flat string match and search
FlatStringMatch(Part 13) and FlatStringSearch(Part 16) are now not-inlinable
not sure if they can be inlined (they're basically same as String.prototype.indexOf)


Also, String.prototype.split(regexp), String.prototype.replace(regexp/g), and some other functions are still slow
(Assignee)

Comment 28

4 years ago
(In reply to Tooru Fujisawa [:arai] from comment #27)
> * inline flat string match and search
> FlatStringMatch(Part 13) and FlatStringSearch(Part 16) ... (they're basically same as
> String.prototype.indexOf)

just for record, this was wrong.
they check RegExp.prototype properties, and that code takes possibly 30% of total time (removing IsRegExpCallOptimizable call reduces total time to 70%)
so far I cannot find any faster way to check that.

even in selfhosted JS, getting getter function is so slow.
here's WIP code:

  function IsRegExpSearchOptimizable() {
    var proto = global.RegExp.prototype;
    if (proto[std_search] != RegExpSearch)
      return false;
  //  var desc = std_Object_getOwnPropertyDescriptor(proto, "global");

    return true;
  }

  // ES6 21.1.3.15.
  function String_search(regexp) {
  ...
        if (IsString(regexp) && IsRegExpSearchOptimizable()) {
          var flatResult = FlatStringSearch(string, regexp);
          if (flatResult !== -2)
            return flatResult;
        }
  ...
  }

when I uncomment |var desc = std_Object_getOwnPropertyDescriptor(proto, "global");| line, performance gets 5x slower
(Assignee)

Comment 29

4 years ago
Posted image Performance comparison of search (obsolete) —
Calculated following conditions:
  * before the patch
  * after the patch
  * omit StringHasNoSearch
    comment out |&& StringHasNoSearch()| in String.js
  * omit IsRegExpCallOptimizable
    comment out IsRegExpCallOptimizable check in jsstr.cpp
  * omit both
    comment out |&& StringHasNoSearch()| and IsRegExpCallOptimizable check

So, both of StringHasNoSearch and IsRegExpCallOptimizable takes about 30% of the total time.
(Assignee)

Comment 30

4 years ago
one more comparison with selfhosting StringHasNoSearch
  * before the patch
  * after the patch
  * selfhost StringHasNoSearch
    A. function StringHasNoSearch() {
         var StringProto = global.String.prototype;
         var ObjectProto = global.Object.prototype;
         if (callFunction(std_Object_hasOwnProperty, ObjectProto, std_search))
           return false;
         if (callFunction(std_Object_hasOwnProperty, StringProto, std_search))
           return false;
         return true;
       }
    B. function StringHasNoSearch() {
         var StringProto = global.String.prototype;
         var ObjectProto = global.Object.prototype;
         if (std_search in ObjectProto)
           return false;
         if (std_search in StringProto)
           return false;
         return true;
       }
    C. var StringProto = Object.prototype; // String.prototype is not yet available
                                           // just for testing
       var ObjectProto = Object.prototype;
       function StringHasNoSearch() {
         if (callFunction(std_Object_hasOwnProperty, ObjectProto, std_search))
           return false;
         if (callFunction(std_Object_hasOwnProperty, StringProto, std_search))
           return false;
         return true;
       }
    D. var StringProto = Object.prototype; // String.prototype is not yet available
                                           // just for testing
       var ObjectProto = Object.prototype;
       function StringHasNoSearch() {
         if (std_search in ObjectProto)
           return false;
         if (std_search in StringProto)
           return false;
         return true;
       }

If we can grab String.prototype first, it gets near 20% faster, with C above
(Assignee)

Comment 31

4 years ago
Thanks to jandem, 'in' operator is folded and selfhosted StringHasNoSearch takes almost no time :)
will try searching a way to get correct String.prototype value.
(Assignee)

Comment 32

4 years ago
Update from IRC and bug 1226762.

In above patch, StringHasNoSearch doesn't check StringProto.__proto__, so it's buggy and fixed version is a little slower than comment #31.

Thanks to till, patch in bug 1226762's solved the issue about getting correct String and Object prototypes.
current performance is in bug 1226762 comment #5 (attachment 8690358 [details])

Now I'm trying to selfhost IsRegExpCallOptimizable and to jit checking RegExp.prototype properties there (getters by its shape, other properties by adding GetOwnPropertyPure).
(Assignee)

Comment 33

4 years ago
selfhosted IsRegExpCallOptimizable as IsRegExpSearchOptimizable.

RegExpSearchOptimizable builtin function used there checks following:
  * RegExp.prototype.global getter is not modified
  * RegExp.prototype.sticky getter is not modified
  * RegExp.prototype.exec is an own data property
  * RegExp.prototype[@@search] is an own data property

latter 2 ensure that wen can read those property with |RegExpProto.exec| and |RegExpProto[std_search]| form, without invoking any getter, that helps jitting those code.

iiuc, in JIT, those 4 items can be checked by comparing shape, so inlined RegExpSearchOptimizable checks only RegExp.prototype's shape  (for now, inlined code is not updated when shape is changed.  this needs to be fixed).

so, in this way, we could get nice performance without implementing GetOwnPropertyPure.
now fixed version's gradient in ion-execution is almost same as m-c :)

will post patches in next comment.
(Assignee)

Comment 34

4 years ago
Posted file WIP patches (obsolete) —
fixed IsString in bug 887016 part 3 and 6,
imported jandem's and till's patches, and selfhosted StringHasNoSearch (Part 13) and IsRegExpSearchOptimizable (Part 14).
now match is broken because of missing IsRegExpMatchOptimizable (will implement after some more tests with search)
Attachment #8689450 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Posted image String.prototype.search slow path (obsolete) —
in slow path, over than 50% of time is consumed inside RegExpCreate.
(Assignee)

Comment 36

4 years ago
Posted file WIP patches (obsolete) —
Fixed String.prototype.match, inlined all StringHasNo*, and Re-ordered patches.

Now checking the performance of String.prototype.replace(string, string) case
will post the result in next comment
Attachment #8690388 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Calculated performance of String.prototype.replace(string, string) case, with following configuration:

  * before patch

  * current patch (Part 18)
    without Part 19
    StringHasNoReplace is following

      function StringHasNoReplace() {
        var ObjectProto = GetBuiltinPrototype("Object");
        var StringProto = GetBuiltinPrototype("String");
        if (StringProto.__proto__ != ObjectProto)
          return false;
        if (std_replace in StringProto)
          return false;
        return true;
      }

  * omit StringProto.__proto__ check (Part 18)
    do not check StringProto.__proto__ == ObjectProto

      function StringHasNoReplace() {
        var ObjectProto = GetBuiltinPrototype("Object");
        var StringProto = GetBuiltinPrototype("String");
        if (std_replace in StringProto)
          return false;
        return true;
      }

  * omit StringHasNoReplace (Part 18)
    do not call StringHasNoReplace()

    if (!(IsString(searchValue)/* && StringHasNoReplace()*/) &&
        searchValue !== undefined && searchValue !== null)

  * MIsStringProtoObject (Part 19)
    checking the performance in JIT code.
    StringHasNoReplace is following.

      function StringHasNoReplace() {
        var StringProto = GetBuiltinPrototype("String");
        if (!IsStringProtoObject(StringProto))
          return false;
        if (std_replace in StringProto)
          return false;
        return true;
      }

    IsStringProtoObject is an inlinable selfhost builtin, that just returns 1 in Ion exectuion

      void
      CodeGenerator::visitIsStringProtoObject(LIsStringProtoObject* ins)
      {
          //Register object = ToRegister(ins->object());
          Register output = ToRegister(ins->output());

          masm.move32(Imm32(1), output);
      }

  * MConstant (Part 19)
    StringHasNoReplace is same as above. but generates MConstant instruction
    instead of IsStringProtoObject instruction

      // MInstruction* cte = MIsStringProtoObject::New(alloc(), callInfo.getArg(0));
      // current->add(cte);
      // current->push(cte);
      pushConstant(BooleanValue(true));

Gradient in Ion execution depends on following:

  * the existence of |StringProto.__proto__ != ObjectProto| check in StringHasNoReplace
  * whether IsStringProtoObject is MConstant or not

So, looks like it's not because |StringProto.__proto__ != ObjectProto| is slow.  maybe, some optimization is not applied when there is non-constant instruction?
(Assignee)

Comment 38

4 years ago
(In reply to Tooru Fujisawa [:arai] from comment #36)
> Created attachment 8690469 [details]
> WIP patches
> 
> Fixed String.prototype.match, inlined all StringHasNo*

s/inlined/self-hosted/
(Assignee)

Updated

4 years ago
Depends on: 1226936
(Assignee)

Comment 39

4 years ago
RegExp.prototype.replace(regexp/g, func) case is now improved by making specialized branch for nCaptures <= 4 cases (4 has no meaning, could be more, i guess), and nCaptures > 4 cases also be improved by pushing |matched| first instead of unshifting it later:

        if (functionalReplace && nCaptures > 4)
            callFunction(std_Array_push, captures, matched);

        // Step 16.l.
        while (n <= nCaptures) {
            ....
        }

        // Step 16.m.
        var replacement;
        if (functionalReplace) {
            if (nCaptures == 0) {
                replacement = ToString(replaceValue(matched, position, S));
            } else if (nCaptures == 1) {
                replacement = ToString(replaceValue(matched, captures[0], position, S));
            } else if (nCaptures == 2) {
                replacement = ToString(replaceValue(matched, captures[0], captures[1],
                                                    position, S));
            } else if (nCaptures == 3) {
                replacement = ToString(replaceValue(matched, captures[0], captures[1],
                                                    captures[2], position, S));
            } else if (nCaptures == 4) {
                replacement = ToString(replaceValue(matched, captures[0], captures[1],
                                                    captures[2], captures[3], position, S));
            } else {
                // Steps 16.m.ii-v, 16.o.
                callFunction(std_Array_push, captures, position);
                callFunction(std_Array_push, captures, S);
                replacement = ToString(callFunction(std_Function_apply, replaceValue, null, captures));
            }
        }

remaining notable performance issues are String.prototype.replace(string, string) case and String.prototype.split(regexp) case.
(Assignee)

Updated

4 years ago
Attachment #8690470 - Attachment description: Performance comparison of replace → Performance comparison of replace(string, string)
(Assignee)

Comment 40

4 years ago
For String.prototype.split(regexp) case, it takes so much time to call RegExpExec for all characters, with sticky flag.
It can be improved by using with current m-c way [1], so, executing RegExp without sticky flag.
of course this optimization can be applied only if all operation are not visible.  the condition is almost same as match/search, but this time we need to check following:
  * constructor returned from |this| is RegExp
  * @@split and exec are not modified
  * global and sticky are not modified

also, this can be applied after creating splitter with given constructor, otherwise we need to check more properties and prototypes of |this|.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#3674
(Assignee)

Comment 41

4 years ago
maybe I'm using inappropriate testcase for String.prototype.replace(string,string) and replace operation is optimized away on m-c?

I used following testcase, so, it returns same value:
  let n = 0;
  let s = ("hello, world!").repeat(10);
  let r = "world";
  let repl = "goodbye";
  function f(i) {
    n += s.replace(r, repl).length;
  }
  let T = elapsed();
  for (let i = 0; i < 50000; i ++)
    f(i);
  print(elapsed() - T);

if I changed it to following, there is no notable difference between m-c and patched one
  let n = 0;
  let ss = [];
  for (let i = 0; i < 50000; i++) {
    ss[i] = i + ("hello, world!").repeat(10);
  }
  let r = "world";
  let repl = "goodbye";
  function f(i) {
    n += ss[i].replace(r, repl).length;
  }
  let T = elapsed();
  for (let i = 0; i < 50000; i ++)
    f(i);
  print(elapsed() - T);
(Assignee)

Comment 42

4 years ago
Posted file WIP patches (obsolete) —
Updated for comment #39 and comment #40.
will post performance comparison with two cases in comment #41.
Attachment #8690469 - Attachment is obsolete: true
(Assignee)

Comment 43

4 years ago
this is the graph for different input for each loop.
(Assignee)

Comment 44

4 years ago
this is the graph for same (constant) input for each loop.
(Assignee)

Comment 45

4 years ago
oh, forgot to change the title in the graph.  "stored2" is "after".

fwiw, here are dromaeo string and regexp test results with current WIP patches:
  before: http://dromaeo.com/?id=243496
  after:  http://dromaeo.com/?id=243497
(In reply to Tooru Fujisawa [:arai] from comment #43)
> Created attachment 8690644 [details]
> Performance comparison of match/search/replace/split/exec/test, with
> different input
> 
> this is the graph for different input for each loop.

I've took some time to look at "search(string)". The issue I found is with "RegExpCreate", which is quite heavy.
Before:
- the code tried to avoid creating RegExpObjects. E.g. for search/match/... a RegExpShared was used. That had all information to match, but is cached. I.e. we didn't have to create this object every time.
- In RegExpCreate we call RegExpInitialize, which also does 'irregexp::ParsePatternSyntax' every time, instead of e.g. relying on RegExpShared to get the cached result.

Possibly that is the slowdown we see on str.match(str) and str.search(str)
(Assignee)

Comment 47

4 years ago
Posted file WIP patches (obsolete) —
improved following:
  * match(pattern-string) and search(pattern-string)
    changed RegExpCreate to use RegExpShared and allocate RegExpObject as GenericObject,
    also call RegExpMatcher and RegExpTester directly is optimizable

  * search(regexp)
    Add RegExpExecForSearch, that is almost same as RegExpExec but calls RegExpBuiltinExecForTest.
    RegExpBuiltinExecForTest is almost same as RegExpBuiltinExec but calls RegExpTester, that does not make match object
Attachment #8690643 - Attachment is obsolete: true

Updated

4 years ago
Blocks: 1227906
(Assignee)

Updated

3 years ago
No longer blocks: 1227906
(Assignee)

Updated

3 years ago
Depends on: 1207922
(Assignee)

Comment 50

3 years ago
just noticed that optimization for search in comment #47 was wrong.
RegExpTester returns the value of lastIndex, not the beginning of the match.
we might need another mode for regexp execution inside NativeRegExpMacroAssembler::GenerateCode to optimize search.
(not sure it worth doing so tho...)
(Assignee)

Updated

3 years ago
Depends on: 1240353
(Assignee)

Comment 51

3 years ago
for @@search, we could create and use a stub that returns the beginning of the match, only if rx.lastIndex property is not an accessor.
will investigate the performance improvement and additional complexity.

anyway, we will need a way to check RegExp instance's own property descriptors (maybe, including __proto__) with low cost (hopefully shape comparison) if we need to optimize away property and flags access.

[1] https://tc39.github.io/ecma262/#sec-regexp.prototype-@@search
(Assignee)

Comment 52

3 years ago
try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07d1dddde44b

Added RegExpSearcher function that returns the start and end indices of the match, in encoded single int32 value (15bit + 15bit).
it can be used in @@replace and @@search, so that we don't have to allocate match result object nor dependent strings inside it.

Also, added RegExpInstanceOptimizable function that checks given RegExp instance has no additional properties, and its prototype is RegExp.prototype.  It's done with shape comparison.
if RegExp.prototype is also not modified, all RegExp-related operations can be optimized, such like skipping, reordering, merging multiple call/access to single, etc.


Current octane regexp score is following:

before (today's m-c): 3735
before (today's m-i): 3840 (with bug 1240353 patch)
after:                3507 (-9% from m-i)

I hope it's almost in acceptable range.
Will ask review after try run (if passed!) and some more tuning :)
(Assignee)

Comment 53

3 years ago
here's current performance comparison result.
Attachment #8689451 - Attachment is obsolete: true
Attachment #8689452 - Attachment is obsolete: true
Attachment #8689971 - Attachment is obsolete: true
Attachment #8689977 - Attachment is obsolete: true
Attachment #8690010 - Attachment is obsolete: true
Attachment #8690387 - Attachment is obsolete: true
Attachment #8690399 - Attachment is obsolete: true
Attachment #8690470 - Attachment is obsolete: true
Attachment #8690518 - Attachment is obsolete: true
Attachment #8690524 - Attachment is obsolete: true
Attachment #8691842 - Attachment is obsolete: true
Attachment #8691843 - Attachment is obsolete: true
Attachment #8691844 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1244098
(Assignee)

Comment 54

3 years ago
Prepared 25 patches (+1 for temporary fix)

Parts 1-9 adds utility functions, and some preparation for optimization.
Part 10-21 adds each method/property.
Part 22-24 adds another optimization with new stub.
Part 25 fixed test.

[part 1]

RegExpCreate is called mostly for creating temporary RegExp object, and could be called with same parameter several times, so it uses GenericObject and RegExpShared, to improve performance.

Added RegExpSharedUse parameter to RegExpInitialize, the default value is DontUseRegExpShared, that is same behavior as before.

Also added NewObjectKind parameter to RegExpAlloc, the default value is TenuredObject, that is also same behavior as before.

https://tc39.github.io/ecma262/#sec-regexpcreate
Attachment #8713617 - Flags: review?(hv1989)
(Assignee)

Comment 55

3 years ago
[Part 2]

just a wrapper for native RegExpCreate.
Attachment #8713618 - Flags: review?(till)
(Assignee)

Comment 56

3 years ago
[Part 3]

IsString is used to optimize string case in String.prototype.match etc.

Is the type check in IonBuilder::inlineIsString correct?
I'd like to inline both "absolutely-string" and "absolutely-non-string" cases.
Attachment #8713619 - Flags: review?(jdemooij)
(Assignee)

Comment 57

3 years ago
[Part 4]

LookupOwnPropertyPure does LookupPropertyPure only one loop, without returning *objp.
used in Part 5 and 6.
Attachment #8713620 - Flags: review?(jdemooij)
(Assignee)

Comment 58

3 years ago
[Part 5]

GetOwnNativeGetterPure is a variant of GetPropertyPure that returns native function of getter accessor.
used by RegExpPrototypeOptimizable in Part 7 to check RegExp.prototype accessors.
Attachment #8713621 - Flags: review?(jdemooij)
(Assignee)

Comment 59

3 years ago
[Part 6]

HasOwnDataPropertyPure is used by RegExpPrototypeOptimizable in Part 7 to check RegExp.prototype data property.
Attachment #8713622 - Flags: review?(jdemooij)
(Assignee)

Comment 60

3 years ago
[Part 7]

RegExpPrototypeOptimizable checks if RegExp.prototype is not modified and spec steps related to RegExp.prototype can be optimized.

Following accessors are checked directly in RegExpPrototypeOptimizable.
  * RegExp.prototype.global
  * RegExp.prototype.sticky

Following data properties are checked only if it's data property.
actual value is checked in self-hosted JS code, as those values are self-hosted functions.
  * RegExp.prototype[@@match]
  * RegExp.prototype[@@search]
  * RegExp.prototype.exec

code for @@search is now commented out because it's not yet added (uncommented in Part 12)

in inlined JIT code, it's checked by shape comparison, with cached shape in RegExpCompartment.
Attachment #8713623 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 61

3 years ago
[Part 8]

RegExpInstanceOptimizable checks if RegExp instance has only default propeties and it's prototype is RegExp.prototype, so that spec steps related to RegExp object can be optimized.  (also requires RegExpPrototypeOptimizable)

It's checked by following conditions (am I correct?)
  * rx.lastIndex is the lastProperty
  * rx's prototype is RegExp.prototype

in inlined JIT code, this is also checked by shape comparison.
Attachment #8713624 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 62

3 years ago
Posted patch Part 9: Add ProtoEquals. (obsolete) — Splinter Review
[Part 9]

ProtoEquals is used to fold |String.prototype.__proto__ == Object.prototype| comparison into boolean constant.
|ProtoEquals(obj, proto)| is folded to boolean value if obj and proto are singleton native objects, and have cacheable proto.

This is needed to apply split+join optimization.
Attachment #8713626 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 63

3 years ago
[Part 10]

RegExp.prototype[@@match] is implemented in pretty straight forward way.
No tricky optimization is applied.

https://tc39.github.io/ecma262/#sec-regexp.prototype-@@match
Attachment #8713627 - Flags: review?(till)
(Assignee)

Comment 64

3 years ago
[Part 11]

String.prototype.match has some optimization paths.

1. StringHasNoMatch checks if string value does have @@match property, that is used to avoid accessing string[@@match], that is high-cost.

2. IsStringMatchOptimizable checks if RegExp.prototype[@@match] on newly created RegExp object is not observed by user script.
2a. If passed argument is string, FlatStringMatch tries to apply flat match.  it fails when pattern is long or it contains meta char, and returns undefined.
2b. If FlatStringMatch fails but IsStringMatchOptimizable is true, we can optimize away all operations for newly created RegExp object, and the state of the RegExp object doesn't matter, as it's thrown away instantly, so it calls RegExpMatcher directly.

non-standard flags argument is still supported, and added WarnOnceAboutFlagsArgument to report warning.

https://tc39.github.io/ecma262/#sec-string.prototype.match
Attachment #8713628 - Flags: review?(till)
(Assignee)

Comment 65

3 years ago
Posted patch Part 12: Add Symbol.search. (obsolete) — Splinter Review
[Part 12]

Just adds Symbol.search, and uncomment @@search code for RegExpPrototypeOptimizable.

https://tc39.github.io/ecma262/#sec-symbol.search
Attachment #8713629 - Flags: review?(till)
(Assignee)

Comment 66

3 years ago
[Part 13]

RegExp.prototype[@@search] is also implemented in pretty straight forward way.
No tricky optimization is applied in this patch.

Optimization is applied in Part 24, after adding RegExpSearcher stub.

https://tc39.github.io/ecma262/#sec-regexp.prototype-@@search
Attachment #8713630 - Flags: review?(till)
(Assignee)

Comment 67

3 years ago
[Part 14]

String.prototype.search has almost same optimization path as String.prototype.match.

non-standard flags argument is still supported.

https://tc39.github.io/ecma262/#sec-string.prototype.search
Attachment #8713631 - Flags: review?(till)
(Assignee)

Comment 68

3 years ago
Posted patch Part 15: Add Symbol.replace. (obsolete) — Splinter Review
[Part 15]

Just adds Symbol.replace.

https://tc39.github.io/ecma262/#sec-symbol.replace
Attachment #8713632 - Flags: review?(till)
(Assignee)

Comment 69

3 years ago
[Part 16]

RegExp.prototype[@@replace] has some optimization paths.

1. IsRegExpReplaceOptimizable checks if operations inside @@replace is not observed.

2. if replaceValue is string and it has no "$" character, and IsRegExpReplaceOptimizable is true, we need only |result.index| and |result[0].length|, no need to handle captures nor creating results array.

3. In functionalReplace, added dedicated path for |0 <= nCaptures <= 4| cases, to avoid calling std_Function_apply.

4. If replaceValue is string and it has no "$" character, we don't need to calculate captures array (but need to apply ToString to each captures)


GetSubstitution is implemented in native function, as it seems to be faster than self-hosted JS.
some parts are copied from jsstr.cpp.  jsstr.cpp's code will be simplified in Part 17.

More optimization is applied in Part 23.

https://tc39.github.io/ecma262/#sec-regexp.prototype-@@replace
Attachment #8713633 - Flags: review?(till)
(Assignee)

Comment 70

3 years ago
[Part 17]

String.prototype.replace has following optimizations.

1. StringHasNoReplace, that is same way as match/search, to avoid accessing searchValue[@@replace] when searchValue is a string.

2. when replaceValue is a string, apply replace in native function and JIT, that reuses current String.prototype.replace optimized path.

3. when replaceValue is not callable, this is almost same path as 2.  separated because IsString can be folded into boolean, but IsCallable is not.  so if replaceValue is a string, path 2 is faster.


loop in js/src/jit-test/tests/ion/bug977966.js is increased because it needs more cycles to catch error case more frequently.

in Ion, RegExpReplace instruction is removed.  StrReplace and StringReplace are merged.

non-standard flags argument is still supported, and it needs special handling, because we ignore all meta chars but ignoreCase flag is effectful.  so added RegExpEscape function to escape all meta chars.  if flags is specified, it compiles escaped pattern as RegExp, and calls @@replace.

https://tc39.github.io/ecma262/#sec-string.prototype.replace
Attachment #8713636 - Flags: review?(till)
Attachment #8713636 - Flags: review?(hv1989)
(Assignee)

Comment 71

3 years ago
[Part 18]

Added RegExp[@@species] to use it in @@split.
It's already reviewed in bug 1131043. delayed to align with the @@species support in @@split.

https://tc39.github.io/ecma262/#sec-get-regexp-@@species
Attachment #8713637 - Flags: review+
(Assignee)

Comment 72

3 years ago
Posted patch Part 19: Add Symbol.split. (obsolete) — Splinter Review
[Part 19]

Just adds Symbol.split.

https://tc39.github.io/ecma262/#sec-symbol.split
Attachment #8713638 - Flags: review?(till)
(Assignee)

Comment 73

3 years ago
[Part 20]

RegExp.prototype[@@split] has following optimization.

1. IsRegExpSplitOptimizable checks if operations inside @@split is not observed.  if it's true, optimize away most operations on newly created RegExp object.

str_includes is exposed to self-hosted global to check flags.

https://tc39.github.io/ecma262/#sec-regexp.prototype-@@split
Attachment #8713639 - Flags: review?(till)
(Assignee)

Comment 74

3 years ago
[Part 20.1]

Currently SpeciesConstructor does not get @@species, as TypedArrays don't support it (bug 1165053).
So added dedicated function until it gets fixed.

if bug 1165053 patch gets landed first, this patch will be removed.

https://tc39.github.io/ecma262/#sec-speciesconstructor
Attachment #8713643 - Flags: review?(till)
(Assignee)

Comment 75

3 years ago
[Part 21]

String.prototype.split has following optimizations.

1. StringHasNoSplit, that is same way as others, to avoid accessing splitter[@@split] when splitter is a string.

2. some steps are reordered to make split+join optimization works.  Now inlineable function is StringSplitString, that is called at the end of String.prototype.split.  split+join optimization in JIT can be applied only if whole function is simple enough.

https://tc39.github.io/ecma262/#sec-string.prototype.split
Attachment #8713644 - Flags: review?(till)
Attachment #8713644 - Flags: review?(hv1989)
(Assignee)

Comment 76

3 years ago
Posted patch Part 22: Add RegExpSearcher. (obsolete) — Splinter Review
[Part 22]

This is another optimization, RegExpSearcher is a variant of RegExpMatcher Ion stub, that returns the start and end indices of the match, in encoded single int32 value (15bit + 15bit), so that we can avoid allocating object/strings to return those 2 values, needed by @@replace and @@search.

I guess 15bit is enough for most cases.  we might be able to use whole 64bit of floating number, but it will make consumers complicated.

The code itself is a mix of RegExpMatcher and RegExpTester.
Attachment #8713647 - Flags: review?(hv1989)
(Assignee)

Comment 77

3 years ago
[Part 23]

Applies optimization to @@replace.
if @@replace is called with string without "$" character, and all operations are not observed, we need only 2 information, the start and end index, for each match.
Attachment #8713648 - Flags: review?(till)
(Assignee)

Comment 78

3 years ago
[Part 24]

Applies optimization to @@search
actually search does not need the end index, as lastIndex is restored to previous value, but re-using RegExpSearcher for simplicity.
Attachment #8713649 - Flags: review?(till)
(Assignee)

Comment 79

3 years ago
[Part 25]

sunspider/check-string-unpack-code.js is still hitting timeout on cgc with --ion-eager.


Green on try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa3e7a79550c (failed some tests)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=150be14d03fb (fixed)
Attachment #8713651 - Flags: review?(till)
Comment on attachment 8713620 [details] [diff] [review]
Part 4: Add LookupOwnPropertyPure.

Review of attachment 8713620 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +2341,5 @@
>  }
>  
> +bool
> +js::LookupOwnPropertyPure(ExclusiveContext* cx, JSObject* obj, jsid id, Shape** propp)
> +{

Add AutoCheckCannotGC in this function.
Comment on attachment 8713621 [details] [diff] [review]
Part 5: Add GetOwnNativeGetterPure.

Review of attachment 8713621 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +2429,5 @@
>  }
>  
>  bool
> +js::GetOwnNativeGetterPure(JSContext* cx, JSObject* obj, jsid id, JSNative* native)
> +{

Add AutoCheckCannotGC in this function.
Comment on attachment 8713623 [details] [diff] [review]
Part 7: Add RegExpPrototypeOptimizable.

Review of attachment 8713623 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +1080,5 @@
> +js::RegExpPrototypeOptimizable(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 1);
> +    MOZ_ASSERT(args[0].isObject());

Add a comment that this function can only be called from self-hosted code.
This assertion is redundant with the toObject() which is below, either remove it or make these assertion be a MOZ_RELEASE_ASSERT().

@@ +1092,5 @@
> +}
> +
> +bool
> +js::RegExpPrototypeOptimizableRaw(JSContext* cx, JSObject* proto, bool* result)
> +{

As this function is not called with a callVM, this function is not allowed to do any GC, as no safepoints are available.

Add a AutoCheckCannotGC to this function.

::: js/src/jit/CodeGenerator.cpp
@@ +2015,5 @@
> +    Register temp = ToRegister(ins->temp());
> +
> +    saveVolatile(output);
> +
> +    masm.adjustStack(-int32_t(sizeof(Value)));

nit: masm.reserveStack(sizeof(Value))

@@ +2027,5 @@
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, RegExpPrototypeOptimizableRaw));
> +    masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
> +
> +    masm.load32(Address(masm.getStackPointer(), 0), output);
> +    masm.adjustStack(sizeof(Value));

nit: masm.freeStack.

@@ +2030,5 @@
> +    masm.load32(Address(masm.getStackPointer(), 0), output);
> +    masm.adjustStack(sizeof(Value));
> +    // C++ compilers like to only use the bottom byte for bools, but we need to
> +    // maintain the entire register.
> +    masm.and32(Imm32(0xFF), output);

Why is that needed?

::: js/src/jit/MIR.h
@@ +7662,5 @@
> +        setMovable();
> +    }
> +
> +  public:
> +    INSTRUCTION_HEADER(RegExpPrototypeOptimizable)

follow-up: Sounds to me that it might make sense to have a foldsTo function to replace this instruction by a MConstant when the Object input is a constant.

This would help UCE to get rid of unnecessary branches.

::: js/src/vm/RegExpObject.cpp
@@ +709,5 @@
>  
>  /* RegExpCompartment */
>  
>  RegExpCompartment::RegExpCompartment(JSRuntime* rt)
> +  : set_(rt), matchResultTemplateObject_(nullptr), optimizableRegExpPrototypeShape_(nullptr)

nit: move each definition to it own line.
Attachment #8713623 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8713624 [details] [diff] [review]
Part 8: Add RegExpInstanceOptimizable.

Review of attachment 8713624 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +1173,5 @@
> +}
> +
> +bool
> +js::RegExpInstanceOptimizableRaw(JSContext* cx, JSObject* rx, JSObject* proto, bool* result)
> +{

Same thing as RegExpPrototypeOptimizableRaw.

Add a AutoCheckCannotGC to this function.

::: js/src/vm/RegExpObject.h
@@ +437,5 @@
>      /* Accessors. */
>  
>      static unsigned lastIndexSlot() { return LAST_INDEX_SLOT; }
>  
> +    static bool isInitialShape(JSContext* cx, NativeObject* nobj) {

nit: cx is not used in this function.
Attachment #8713624 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8713626 [details] [diff] [review]
Part 9: Add ProtoEquals.

Review of attachment 8713626 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MCallOptimize.cpp
@@ +1716,5 @@
> +        return InliningStatus_NotInlined;
> +    }
> +
> +    MDefinition* objArg = convertUnboxedObjects(callInfo.getArg(0));
> +    MDefinition* protoArg = convertUnboxedObjects(callInfo.getArg(1));

These functions are adding Guard instructions which would not be removed by DCE.  Thus if we are not able to inline this function, then we would still have these guard instructions which might cause early failures with no benefit.

We should only add new instruction to the graph after the last "return NotInlined;" statement.
Attachment #8713626 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 85

3 years ago
Thank you for reviewing :D

(In reply to Nicolas B. Pierron [:nbp] from comment #82)
> @@ +2030,5 @@
> > +    masm.load32(Address(masm.getStackPointer(), 0), output);
> > +    masm.adjustStack(sizeof(Value));
> > +    // C++ compilers like to only use the bottom byte for bools, but we need to
> > +    // maintain the entire register.
> > +    masm.and32(Imm32(0xFF), output);
> 
> Why is that needed?

for example, RegExpInstanceOptimizableRaw returns 0xE5E5E500 as false.
so we need to mask with 0xFF.


> ::: js/src/jit/MIR.h
> @@ +7662,5 @@
> > +        setMovable();
> > +    }
> > +
> > +  public:
> > +    INSTRUCTION_HEADER(RegExpPrototypeOptimizable)
> 
> follow-up: Sounds to me that it might make sense to have a foldsTo function
> to replace this instruction by a MConstant when the Object input is a
> constant.
> 
> This would help UCE to get rid of unnecessary branches.

would you tell me what "Object input is a constant" means here?
it's used like following:

>     var RegExpProto = GetBuiltinPrototype("RegExp");
>     if (!RegExpPrototypeOptimizable(RegExpProto))
>         return false;
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 86

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #84)
> Comment on attachment 8713626 [details] [diff] [review]
> Part 9: Add ProtoEquals.
> 
> Review of attachment 8713626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +1716,5 @@
> > +        return InliningStatus_NotInlined;
> > +    }
> > +
> > +    MDefinition* objArg = convertUnboxedObjects(callInfo.getArg(0));
> > +    MDefinition* protoArg = convertUnboxedObjects(callInfo.getArg(1));
> 
> These functions are adding Guard instructions which would not be removed by
> DCE.  Thus if we are not able to inline this function, then we would still
> have these guard instructions which might cause early failures with no
> benefit.

sorry, actually I didn't understand the code related to guards.
I have some questions.

  1. could builtin constructor and prototype be unboxed object?
  2. if I just remove the convertUnboxedObjects, what could be wrong?
  3. how are the checks done in inlineProtoEquals guaranteed for subsequent call if we replace the function with bool constant?
     so, what happens if the prototype is modified?
(In reply to Tooru Fujisawa [:arai] from comment #86)
>   1. could builtin constructor and prototype be unboxed object?

Unboxed objects can only be created for objects with the PlainObject class and unboxed arrays can only be created for objects with the ArrayObject class. Furthermore, unboxed objects will be converted to native objects when they're used as the prototype of another objects.

>   2. if I just remove the convertUnboxedObjects, what could be wrong?

In this case, nothing. The code in ProtoEquals (what do you think about calling this ObjectHasPrototype?) does the right thing for unboxed objects.

>   3. how are the checks done in inlineProtoEquals guaranteed for subsequent
> call if we replace the function with bool constant?
>      so, what happens if the prototype is modified?

When an object's proto changes, the object's group is marked as having unknown properties. When you call hasStableClassAndProto(constraints), you add a type constraint that will invalidate the code when the object gets unknown properties.
That said, these patches need lots of tests :)

It'd be great if we could add jit-tests for the intrinsics themselves, and also for the functions that use them, to make sure they do the right thing when you change prototypes, builtin properties, etc.
(Assignee)

Comment 89

3 years ago
Thank you for detailed explanation :D

added jittest for IsString.
Attachment #8714028 - Flags: review?(jdemooij)
(Assignee)

Comment 90

3 years ago
Removed convertUnboxedObjects, renamed ProtoEquals to ObjectHasPrototype, as suggested, and added testcase.
Attachment #8714029 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

3 years ago
Attachment #8713626 - Attachment is obsolete: true
(Assignee)

Comment 91

3 years ago
Forgot to include basic testcase for String.prototype.match.
Also, added jittest for inlineable functions used inside it.
Attachment #8714030 - Flags: review?(till)
Comment on attachment 8713618 [details] [diff] [review]
Part 2: Add self-hosted RegExpCreate wrapper.

Review of attachment 8713618 [details] [diff] [review]:
-----------------------------------------------------------------

Please change the patch description to "Add RegExpCreate self-hosting intrinsic." The function is not itself self-host*ed*, but part of the self-host*ing* infrastructure.
Attachment #8713618 - Flags: review?(till) → review+
Comment on attachment 8713627 [details] [diff] [review]
Part 10: Implement RegExp.prototype[@@match].

Review of attachment 8713627 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice. R=me with or without the suggested changes in RegExp.js.

::: js/src/builtin/RegExp.js
@@ +138,5 @@
> +        _DefineDataProperty(A, n, matchStr);
> +
> +        // Step 6.e.iii.4.
> +        if (matchStr === "")
> +            rx.lastIndex = AdvanceStringIndex(S, ToLength(rx.lastIndex), fullUnicode);

It probably doesn't make a meaningful difference, but theoretically, you could split up AdvanceStringIndex into an ASCII and a unicode version and call the right one here depending on the value of fullUnicode. Or even replace the fullUnicode boolean with something like
var advanceStringIndex = !rx.unicode ? ASCIIAdvanceStringIndex : UnicodeAdvanceStringIndex;

The advantage would be that the ASCII version would be less heavy and thus quicker to inline.

Going even further, you could also remove step 4 from AdvanceStringIndex and use something like this here:
var lastIndex = ToLength(rx.lastIndex);
rx.lastIndex = fullUnicode ? AdvanceStringIndex(S, lastIndex) : lastIndex + 1;

::: js/src/js.msg
@@ +457,5 @@
>  MSG_DEF(JSMSG_BACK_REF_OUT_OF_RANGE,   0, JSEXN_SYNTAXERR, "back reference out of range in regular expression")
>  MSG_DEF(JSMSG_BAD_CLASS_RANGE,         0, JSEXN_SYNTAXERR, "invalid range in character class")
>  MSG_DEF(JSMSG_DEPRECATED_REGEXP_MULTILINE, 0, JSEXN_SYNTAXERR, "RegExp.multiline is deprecated. Use m flag instead")
>  MSG_DEF(JSMSG_ESCAPE_AT_END_OF_REGEXP, 0, JSEXN_SYNTAXERR, "\\ at end of pattern")
> +MSG_DEF(JSMSG_EXEC_NOT_OBJORNULL,      0, JSEXN_TYPEERR, "exec should return object or null")

Nit: s/exec/RegExp exec method/.
Attachment #8713627 - Flags: review?(till) → review+
Comment on attachment 8713628 [details] [diff] [review]
Part 11: Call RegExp.prototype[@@match] from String.prototype.match.

Review of attachment 8713628 [details] [diff] [review]:
-----------------------------------------------------------------

Very satisfying diff stats :)

::: js/src/builtin/String.js
@@ +10,5 @@
> +  if (!ProtoEquals(StringProto, ObjectProto))
> +    return false;
> +  if (std_match in StringProto)
> +    return false;
> +  return true;

Shorten the last three lines to just
return !(std_match in stringProto);

@@ +51,5 @@
> +    if (arguments.length > 1) {
> +        flags = arguments[1];
> +        WarnOnceAboutFlagsArgument();
> +    } else {
> +      if (IsString(regexp) && IsStringMatchOptimizable()) {

Nit: indentation is off in this block.

@@ +63,5 @@
> +    var rx = RegExpCreate(regexp, flags);
> +
> +    // Step 5 (optimized case).
> +    if (IsStringMatchOptimizable() && !flags)
> +      return RegExpMatcher(rx, S, 0, false);

and here.

::: js/src/jsstr.cpp
@@ +1,1 @@
> +

Nit: this probably got here by accident, please remove.
Attachment #8713628 - Flags: review?(till) → review+
(In reply to Tooru Fujisawa [:arai] from comment #85)
> Thank you for reviewing :D
> 
> (In reply to Nicolas B. Pierron [:nbp] from comment #82)
> > @@ +2030,5 @@
> > > +    masm.load32(Address(masm.getStackPointer(), 0), output);
> > > +    masm.adjustStack(sizeof(Value));
> > > +    // C++ compilers like to only use the bottom byte for bools, but we need to
> > > +    // maintain the entire register.
> > > +    masm.and32(Imm32(0xFF), output);
> > 
> > Why is that needed?
> 
> for example, RegExpInstanceOptimizableRaw returns 0xE5E5E500 as false.
> so we need to mask with 0xFF.

wouldn't it be better to use masm.load8ZeroExtend(Address(…), output) ?

> > ::: js/src/jit/MIR.h
> > > +    INSTRUCTION_HEADER(RegExpPrototypeOptimizable)
> > 
> > follow-up: Sounds to me that it might make sense to have a foldsTo function
> > to replace this instruction by a MConstant when the Object input is a
> > constant.
> > 
> > This would help UCE to get rid of unnecessary branches.
> 
> would you tell me what "Object input is a constant" means here?
> it's used like following:
> 
> >     var RegExpProto = GetBuiltinPrototype("RegExp");
> >     if (!RegExpPrototypeOptimizable(RegExpProto))
> >         return false;

When we do a load, and baseline records that the loaded value is always the same, we should replace the the value by a MConstant with the returned object, and add a guard if TI is not able to prove that this loaded value is an upper-bound of what can be loaded.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8714029 [details] [diff] [review]
Part 9: Add ObjectHasPrototype.

Review of attachment 8714029 [details] [diff] [review]:
-----------------------------------------------------------------

This sounds good to me.
Thanks :)

::: js/src/jit-test/tests/ion/testObjectHasPrototype.js
@@ +24,5 @@
> +  var expected = true;
> +  for (var i = 0; i < 120; i++) {
> +    f(expected);
> +    if (i == 40) {
> +      Object.setPrototypeOf(StringProto, proto);

I recall that setPrototypeOf used to be a killer of any type information we gathered so far. (unless this was only for the global)
Can you double check that ObjectHasPrototype is getting recompiled as we add new inputs.
Attachment #8714029 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 97

3 years ago
thanks.
while testing the recompile, noticed that |with (this) {}| prevents inlining ObjectHasPrototype function call.
I thought it's there to prevent inlining the enclosing function...
will fix the testcase.
(Assignee)

Comment 98

3 years ago
Removed |with(this) {};|
Attachment #8714028 - Attachment is obsolete: true
Attachment #8714028 - Flags: review?(jdemooij)
Attachment #8714541 - Flags: review?(jdemooij)
(Assignee)

Comment 99

3 years ago
also removed |with|.
Attachment #8714030 - Attachment is obsolete: true
Attachment #8714030 - Flags: review?(till)
Attachment #8714542 - Flags: review?(till)
(Assignee)

Comment 100

3 years ago
updated AdvanceStringIndex
Attachment #8713630 - Attachment is obsolete: true
Attachment #8713630 - Flags: review?(till)
Attachment #8714545 - Flags: review?(till)
(Assignee)

Comment 101

3 years ago
fixed indentation and shorten condition for StringHasNoSearch,
and removed wrong optimization that used RegExpTester.
Attachment #8713631 - Attachment is obsolete: true
Attachment #8713631 - Flags: review?(till)
(Assignee)

Updated

3 years ago
Attachment #8714548 - Flags: review?(till)
(Assignee)

Comment 102

3 years ago
fixed AdvanceStringIndex.
Attachment #8714549 - Flags: review?(till)
(Assignee)

Updated

3 years ago
Attachment #8713633 - Attachment is obsolete: true
Attachment #8713633 - Flags: review?(till)
(Assignee)

Comment 103

3 years ago
Fixed indentation and shorten the condition in StringHasNoReplace.
Attachment #8713636 - Attachment is obsolete: true
Attachment #8713636 - Flags: review?(till)
Attachment #8713636 - Flags: review?(hv1989)
Attachment #8714550 - Flags: review?(till)
Attachment #8714550 - Flags: review?(hv1989)
(Assignee)

Comment 104

3 years ago
Fixed AdvanceStringIndex
Attachment #8714553 - Flags: review?(till)
(Assignee)

Updated

3 years ago
Attachment #8713639 - Attachment is obsolete: true
Attachment #8713639 - Flags: review?(till)
(Assignee)

Comment 105

3 years ago
Fixed indentation and shorten the condition for StringHasNoSplit.
Attachment #8713644 - Attachment is obsolete: true
Attachment #8713644 - Flags: review?(till)
Attachment #8713644 - Flags: review?(hv1989)
Attachment #8714558 - Flags: review?(till)
Attachment #8714558 - Flags: review?(hv1989)
Comment on attachment 8713617 [details] [diff] [review]
Part 1: Add native RegExpCreate.

Review of attachment 8713617 [details] [diff] [review]:
-----------------------------------------------------------------

I only don't know why we can now suddenly use GenericObject instead of Tenured.
I would r+ immediately if that was 'Tenured'. Can you explain why GenericObject is allowed?

::: js/src/builtin/RegExp.cpp
@@ +233,5 @@
> +js::RegExpCreate(JSContext* cx, HandleValue patternValue, HandleValue flagsValue,
> +                 MutableHandleValue rval)
> +{
> +    /* Step 1. */
> +    Rooted<RegExpObject*> regexp(cx, RegExpAlloc(cx, nullptr, GenericObject));

Shouldn't this be 'TenuredObject' instead of 'GenericObject'? I thought all regexp are currently Tenured?
I think we eventually want to allow regexp to be nursery allocated, but we are not there yet? Or did I miss some changes?
Attachment #8713617 - Flags: review?(hv1989)
Comment on attachment 8713647 [details] [diff] [review]
Part 22: Add RegExpSearcher.

Review of attachment 8713647 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work!

::: js/src/builtin/RegExp.cpp
@@ +102,5 @@
>  }
>  
> +static int32_t
> +CreateRegExpSearchResult(JSContext* cx, const MatchPairs& matches)
> +{

Can you add a small description?
Something like:
"Fit the start and limit of match into a int32_t"

@@ +104,5 @@
> +static int32_t
> +CreateRegExpSearchResult(JSContext* cx, const MatchPairs& matches)
> +{
> +    uint32_t position = matches[0].start;
> +    uint32_t lastIndex = position + matches[0].length();

Isn't this?
matches[0].limit

please use matches[0].limit

@@ +996,5 @@
>      return RegExpMatcherImpl(cx, regexp, input, lastIndex, sticky,
>                               UpdateRegExpStatics, output);
>  }
>  
> +/* ES6 21.2.5.2.2 steps 3, 11-29, except 15.a.i-ii, 15.c.i.1-2, 18. */

Can you add to this comment,
that this code is inlined in CodeGenerator.cpp generateRegExpSearcherStub and that
changes to this code need to ge reflected in there too.

@@ +1052,5 @@
> +    return true;
> +}
> +
> +/* Separate interface for use by IonMonkey.
> + * This code cannot re-enter Ion code. */

Please format comment like:
/*
 * foooofoooofoo
 * fooooooo
 */
Attachment #8713647 - Flags: review?(hv1989) → review+
(Assignee)

Updated

3 years ago
See Also: → 1245025
Comment on attachment 8714550 [details] [diff] [review]
Part 17: Call RegExp.prototype[@@replace] from String.prototype.replace.

Review of attachment 8714550 [details] [diff] [review]:
-----------------------------------------------------------------

I looked at the jit changes. Nice refactoring and removal of old code here!

::: js/src/jit/IonAnalysis.cpp
@@ -1843,5 @@
>  
> -                // All MRegExp* MIR's don't adjust the regexp.
> -                MDefinition* use = i->consumer()->toDefinition();
> -                if (use->isRegExpReplace())
> -                    continue;

Can we add:
- isRegExpMatcher, isRegExpTester and isRegExpSearcher here?

::: js/src/jsstr.cpp
@@ +1968,3 @@
>  static inline bool
>  IsRegExpMetaChar(char16_t c)
>  {

Isn't IsRegExpMetaChar now unused? Can we remove it?

::: js/src/vm/RegExpObject.h
@@ +561,4 @@
>  EscapeRegExpPattern(JSContext* cx, HandleAtom src);
>  
> +extern JSString*
> +RegExpEscape(JSContext* cx, HandleString src);

We now have
* EscapeRegExpPattern
* RegExpEscape

Which are named similar and the code is also similar. Might be good to try to find a name that better describes what these do and make the difference between both of them more clear? Maybe some comments?
Attachment #8714550 - Flags: review?(hv1989)
(Assignee)

Comment 109

3 years ago
Thank you for reviewing!

(In reply to Hannes Verschore [:h4writer] from comment #106)
> Comment on attachment 8713617 [details] [diff] [review]
> Part 1: Add native RegExpCreate.
> 
> Review of attachment 8713617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only don't know why we can now suddenly use GenericObject instead of
> Tenured.

Here's performance comparison for affected cases.
it only affect if pattern string contains meta char, or longer than 256 chars (so that flat match cannot be performed).  that case doesn't appear in octane/regexp.
There are 10-20% difference, but these cases will be rare, so I guess we could just use TenuredObject as an initial step :)

will post updated patch shortly.
(Assignee)

Comment 110

3 years ago
(In reply to Hannes Verschore [:h4writer] from comment #108)
> Comment on attachment 8714550 [details] [diff] [review]
> Part 17: Call RegExp.prototype[@@replace] from String.prototype.replace.
> 
> Review of attachment 8714550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I looked at the jit changes. Nice refactoring and removal of old code here!
> 
> ::: js/src/jit/IonAnalysis.cpp
> @@ -1843,5 @@
> >  
> > -                // All MRegExp* MIR's don't adjust the regexp.
> > -                MDefinition* use = i->consumer()->toDefinition();
> > -                if (use->isRegExpReplace())
> > -                    continue;
> 
> Can we add:
> - isRegExpMatcher, isRegExpTester and isRegExpSearcher here?

Will add them there.


> ::: js/src/jsstr.cpp
> @@ +1968,3 @@
> >  static inline bool
> >  IsRegExpMetaChar(char16_t c)
> >  {
> 
> Isn't IsRegExpMetaChar now unused? Can we remove it?

It's actually used to check if we can perform flat match.
So I should merge 2 duplicated impls of IsRegExpMetaChar and IsRegExpSyntaxChar into one, and IsRegExpMetaChar was correct, that "-" character should not be there.
I think RegExpObject.cpp/h is better place for its function now.
(Assignee)

Comment 111

3 years ago
Removed newKind parameter from RegExpAlloc, and changed back to always use TenuredObject.
Attachment #8713617 - Attachment is obsolete: true
Attachment #8714718 - Flags: review?(hv1989)
(Assignee)

Comment 112

3 years ago
renamed RegExpEscape to EscapeRegExpMetaChars, as it escapes meta chars, compared to EscapeRegExpPattern (it's spec name) that escapes slash and newline.

added isRegExpMatcher and isRegExpTester to MakeMRegExpHoistable.
will add isRegExpSearcher in Part 22.

moved IsRegExpMetaChar, HasRegExpMetaChars and StringHasRegExpMetaChars from jsstr.cpp to RegExpObject.cpp.
Attachment #8714550 - Attachment is obsolete: true
Attachment #8714550 - Flags: review?(till)
Attachment #8714720 - Flags: review?(till)
Attachment #8714720 - Flags: review?(hv1989)
Comment on attachment 8713619 [details] [diff] [review]
Part 3: Add IsString intrinsic.

Review of attachment 8713619 [details] [diff] [review]:
-----------------------------------------------------------------

Can we use |typeof x === "string"| instead of IsString(x)? If we aren't optimizing that enough, we should probably fix that (but typeof should be fast enough AFAIK).

Note that IsObject is a bit more complicated, because |typeof objectEmulatingUndefined()| is "undefined", but strings don't have such cases.
Attachment #8713619 - Flags: review?(jdemooij)
Comment on attachment 8713620 [details] [diff] [review]
Part 4: Add LookupOwnPropertyPure.

Review of attachment 8713620 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +2340,5 @@
>      return true;
>  }
>  
> +bool
> +js::LookupOwnPropertyPure(ExclusiveContext* cx, JSObject* obj, jsid id, Shape** propp)

I think having this function is useful, but there's some duplication with LookupPropertyPure. Can we make LookupPropertyPure call this new LookupOwnPropertyPure function? :)
Attachment #8713620 - Flags: review?(jdemooij)
Comment on attachment 8713621 [details] [diff] [review]
Part 5: Add GetOwnNativeGetterPure.

Review of attachment 8713621 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +2435,5 @@
> +    if (!LookupOwnPropertyPure(cx, obj, id, &shape))
> +        return false;
> +
> +    if (!shape || !shape->hasGetterObject()) {
> +        *native = nullptr;

Nit: you could add |*native = nullptr;| to the start of the function, so this if-body and the two below don't need braces. But it doesn't really matter and it's fine either way.
Attachment #8713621 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 116

3 years ago
Comment on attachment 8713619 [details] [diff] [review]
Part 3: Add IsString intrinsic.

(In reply to Jan de Mooij [:jandem] from comment #113)
> Comment on attachment 8713619 [details] [diff] [review]
> Part 3: Add IsString intrinsic.
> 
> Review of attachment 8713619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we use |typeof x === "string"| instead of IsString(x)? If we aren't
> optimizing that enough, we should probably fix that (but typeof should be
> fast enough AFAIK).

Yeah, replaced with typeof and it's almost same performance :)
Attachment #8713619 - Attachment is obsolete: true
(Assignee)

Comment 117

3 years ago
Comment on attachment 8714541 [details] [diff] [review]
Part 3.1: Add testcase for IsString.

clearing r?, as IsString is no more needed.
Attachment #8714541 - Attachment is obsolete: true
Attachment #8714541 - Flags: review?(jdemooij)
(Assignee)

Comment 118

3 years ago
Okay, changed LookupPropertyPure to call LookupOwnPropertyPure.
there is one exceptional case in LookupOwnPropertyPure, that it returns true and *propp is set to nullptr, but it should return immediately from LookupPropertyPure.

here is original code.
>            if (obj->is<TypedArrayObject>()) {
>                uint64_t index;
>                if (IsTypedArrayIndex(id, &index)) {
>                    if (index < obj->as<TypedArrayObject>().length()) {
>                        *objp = obj;
>                        MarkDenseOrTypedArrayElementFound<NoGC>(propp);
>                    } else {
>                        *objp = nullptr;
>                        *propp = nullptr;
>                    }
>                    return true;
>                }
>            }


so added isTypedArrayOutOfRange out parameter that is default to nullptr.  and set to true when returning from the else branch of LookupOwnPropertyPure.
Attachment #8713620 - Attachment is obsolete: true
Attachment #8714901 - Flags: review?(jdemooij)
Comment on attachment 8713622 [details] [diff] [review]
Part 6: Add HasOwnDataPropertyPure.

Review of attachment 8713622 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment below addressed.

::: js/src/jsobj.cpp
@@ +2463,5 @@
> +    Shape* shape = nullptr;
> +    if (!LookupOwnPropertyPure(cx, obj, id, &shape))
> +        return false;
> +
> +    *result = shape && shape->hasDefaultGetter();

LookupOwnPropertyPure will return a 0x1 Shape* pointer when |id| is a dense element or non-native property (on an unboxed object for instance). We should really fix that footgun (see also bug 1139474 comment 4), but for now please add a !IsImplicitDenseOrTypedArrayElement(shape) check here, so hasDefaultGetter won't crash.

GetOwnNativeGetterPure in the previous patch needs the same fix (I missed it there, sorry).

Also, I think we should check shape->hasSlot() as well.
Attachment #8713622 - Flags: review?(jdemooij) → review+
Comment on attachment 8714901 [details] [diff] [review]
Part 4: Add LookupOwnPropertyPure.

Review of attachment 8714901 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, thanks!
Attachment #8714901 - Flags: review?(jdemooij) → review+
Attachment #8714718 - Flags: review?(hv1989) → review+
Comment on attachment 8713647 [details] [diff] [review]
Part 22: Add RegExpSearcher.

Review of attachment 8713647 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to add RegExpSearcher to jit::MakeMRegExpHoistable
Comment on attachment 8714720 [details] [diff] [review]
Part 17: Call RegExp.prototype[@@replace] from String.prototype.replace.

Review of attachment 8714720 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the changes! Looks good.
@till: I only looked at c++ changes. I assume you will do the js changes.

::: js/src/builtin/String.js
@@ +110,5 @@
> +    if (arguments.length > 2) {
> +        flags = arguments[2];
> +        WarnOnceAboutFlagsArgument();
> +
> +        var rx = RegExpCreate(EscapeRegExpMetaChars(searchString), flags);

Note:
RegExpCreate is an intrinsic and EscapeRegExpMetaChars too. This means we have to go into c++ twice. It might be better to selfhost EscapeRegExpMetaChars. Seems quite easy and removes the c++ call.
=> Now I think this call is only for non-standard flag that is deprecated. No need to optimize for speed if this is eventually getting removed. If that is not the case and we also use it in a hot path, can you open a followup bug?
Attachment #8714720 - Flags: review?(hv1989) → review+
Comment on attachment 8714558 [details] [diff] [review]
Part 21: Call RegExp.prototype[@@split] from String.prototype.split.

Review of attachment 8714558 [details] [diff] [review]:
-----------------------------------------------------------------

I'll need another iteration to convince myself everything is good.
These are the things I already noticed.

::: js/src/jit/BaselineIC.cpp
@@ +5998,5 @@
>      MOZ_ASSERT(callee.isObject());
>      MOZ_ASSERT(callee.toObject().is<JSFunction>());
>  
> +    RootedString str(cx, args[0].toString());
> +    RootedString sep(cx, args[0].toString());

Shouldn't this be args[1] instead of args[0].
Can you add a test for this?

::: js/src/jit/BaselineIC.h
@@ +2959,5 @@
>  
>    protected:
>      uint32_t pcOffset_;
> +    HeapPtrString expectedStr_;
> +    HeapPtrString expectedSep_;

Thanks. This is better.

::: js/src/jit/MCallOptimize.cpp
@@ -1560,1 @@
>  {

This is an optimization added in bug 688219.
Seems this is an important optimization in real world for packers, jquery and such.
As a result we should definitely not remove it.
Can you add partXXX that implements this again?

::: js/src/jsstr.cpp
@@ +2448,2 @@
>          if (endIndex == lastEndIndex) {
> +            index++;

Why can we remove the AdvanceStringIndex here? We still support unicode here, right?
Attachment #8714558 - Flags: review?(hv1989)
(Assignee)

Updated

3 years ago
See Also: → 1245801
(Assignee)

Comment 124

3 years ago
Thank you for reviewing :D

(In reply to Hannes Verschore [:h4writer] from comment #123)
> ::: js/src/jit/BaselineIC.cpp
> @@ +5998,5 @@
> >      MOZ_ASSERT(callee.isObject());
> >      MOZ_ASSERT(callee.toObject().is<JSFunction>());
> >  
> > +    RootedString str(cx, args[0].toString());
> > +    RootedString sep(cx, args[0].toString());
> 
> Shouldn't this be args[1] instead of args[0].
> Can you add a test for this?

Thank you for pointing it out.
I noticed that stack check code in ICCall_StringSplit::Compiler::generateStubCode was not updated correctly (was checking ThisVal instead of CalleeVal).
Fixed both of them and confirmed it's using optimized stub, locally.


> ::: js/src/jit/MCallOptimize.cpp
> @@ -1560,1 @@
> >  {
> 
> This is an optimization added in bug 688219.
> Seems this is an important optimization in real world for packers, jquery
> and such.
> As a result we should definitely not remove it.
> Can you add partXXX that implements this again?

Okay, will add it back as Part 21.1.


> ::: js/src/jsstr.cpp
> @@ +2448,2 @@
> >          if (endIndex == lastEndIndex) {
> > +            index++;
> 
> Why can we remove the AdvanceStringIndex here? We still support unicode
> here, right?

Now native functions are used only from String.prototype.split, not RegExp.prototype[@@split], so it's for the case that separator is a string, and there is no unicode flag.

https://tc39.github.io/ecma262/#sec-string.prototype.split (step 14.c.i)
(Assignee)

Comment 125

3 years ago
Fixed the index of sep parameter in TryAttachStringSplit.

Also, fixed stack value checking in ICCall_StringSplit::Compiler::generateStubCode,
by defining the depth of each value at the top of function, besides the stack layout figure, and update the depth correctly.
Attachment #8714558 - Attachment is obsolete: true
Attachment #8714558 - Flags: review?(till)
Attachment #8716200 - Flags: review?(till)
Attachment #8716200 - Flags: review?(hv1989)
(Assignee)

Comment 126

3 years ago
IonBuilder::inlineConstantStringSplitString is almost same as IonBuilder::inlineConstantStringSplit, except argument (this+arg(0) vs arg(0)+arg(1)).

Added dedicated path at the top of String_split, to pass |this| and |separater| parameter directly to StringSplitString.

will post performance comparison.
Attachment #8716201 - Flags: review?(hv1989)
(Assignee)

Updated

3 years ago
Attachment #8716200 - Attachment filename: 22-Bug_887016___Part_21__Call_RegExp_protot.patch → Part 21: Call RegExp.prototype[@@split] from String.prototype.split.
(Assignee)

Comment 127

3 years ago
Comment on attachment 8716200 [details] [diff] [review]
Part 21: Call RegExp.prototype[@@split] from String.prototype.split.

oops
Attachment #8716200 - Attachment description: 22-Bug_887016___Part_21__Call_RegExp_protot.patch → Part 21: Call RegExp.prototype[@@split] from String.prototype.split.
Attachment #8716200 - Attachment filename: Part 21: Call RegExp.prototype[@@split] from String.prototype.split. → 22-Bug_887016___Part_21__Call_RegExp_protot.patch
(Assignee)

Comment 128

3 years ago
in split with constant this/separator, gradient of Ion execution is almost same as before, but there is around 3ms offset.

split+join optimization is still available, and it shows almost same performance.
Comment on attachment 8716200 [details] [diff] [review]
Part 21: Call RegExp.prototype[@@split] from String.prototype.split.

Review of attachment 8716200 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineIC.cpp
@@ +5660,5 @@
>          }
>      }
>  
> +    if (native == js::intrinsic_StringSplitString && args.length() == 2 && args[0].isString() &&
> +        args[1].isString())

Style nit: Can you make sure this fit a 100 chars?

@@ +6955,5 @@
>  
> +    // Stack Layout: [ ..., CalleeVal, ThisVal, strVal, sepVal, +ICStackValueOffset+ ]
> +    static const size_t SEP_DEPTH = 0;
> +    static const size_t STR_DEPTH = sizeof(Value);
> +    static const size_t CALLEE_DEPTH = 3 * sizeof(Value);

Nice!
Attachment #8716200 - Flags: review?(hv1989) → review+
Comment on attachment 8716201 [details] [diff] [review]
Part 21.1: Add back optimization for String.prototype.split with string constants.

Review of attachment 8716201 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Upon commit, can you fold this and the previous patch?

::: js/src/builtin/String.js
@@ +233,5 @@
>          return false;
>      return !(std_split in StringProto);
>  }
>  
> +// ES 2016 draft Dec 10, 2015

Why did you remove 21.1.3.17. I think that can stay

@@ +239,5 @@
>      // Step 1.
>      RequireObjectCoercible(this);
>  
> +    // Optimized path for string.split(string), especially when both strings
> +    // are constants.

Can you add a comment that the sequence of if's cannot be put together in order that IonMonkey sees the constant if present. Which should get fixed with bug 1246141

::: js/src/jit/IonBuilder.cpp
@@ +3534,5 @@
>      if (subject->resultTypeSet() && type->equals(subject->resultTypeSet()))
>          return true;
>  
> +    if(type->getKnownMIRType() == MIRType_String && subject->type() == MIRType_String)
> +        return true;

Can you add:
// TODO bug 1064543: Make this check work for every MIRType.

::: js/src/jit/MCallOptimize.cpp
@@ +1569,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    const js::Value* strval = callInfo.getArg(0)->toConstant()->vp();
> +    if (!strval->isString())
> +        return InliningStatus_NotInlined;

stupid request, but can you put the code of strval = callInfo.getArg(0) before sepval = callInfo.getArg(1).
Attachment #8716201 - Flags: review?(hv1989) → review+
Comment on attachment 8713629 [details] [diff] [review]
Part 12: Add Symbol.search.

Review of attachment 8713629 [details] [diff] [review]:
-----------------------------------------------------------------

Please fold this and the next patch into the one that's actually using the symbols before landing. That way, there's not danger of accidentally keeping the symbols should we have to back out the code that uses them. Forgetting to do so could badly screw with feature detection code.
Attachment #8713629 - Flags: review?(till) → review+
Comment on attachment 8713632 [details] [diff] [review]
Part 15: Add Symbol.replace.

Review of attachment 8713632 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but same comment as for part 12 applies.
Attachment #8713632 - Flags: review?(till) → review+
Comment on attachment 8713638 [details] [diff] [review]
Part 19: Add Symbol.split.

Review of attachment 8713638 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, again please fold into the patch that's using this symbol.
Attachment #8713638 - Flags: review?(till) → review+
(Assignee)

Updated

3 years ago
Depends on: 1246575
(Assignee)

Comment 134

3 years ago
Now self-hosted match/search/replace also need to disable flags argument on non-release channel.

this will also merged with Part 11 later.
Attachment #8716891 - Flags: review?(till)
(Assignee)

Comment 135

3 years ago
Attachment #8716892 - Flags: review?(till)
(Assignee)

Comment 136

3 years ago
Attachment #8716893 - Flags: review?(till)
Comment on attachment 8713643 [details] [diff] [review]
Part 20.1: Use dedicated SpeciesConstructor until bug 1165053 is fixed.

Review of attachment 8713643 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'd prefer to do this in a less hacky way. Can't we keep using the current version, and once we enable Symbol.species at all, we enable it for all builtins? Or am I overlooking something and the rest of the changes here somehow don't work without the species support?
Attachment #8713643 - Flags: review?(till)
Comment on attachment 8713648 [details] [diff] [review]
Part 23: Use RegExpSearcher in RegExp.prototype[@@replace] optimized path.

Review of attachment 8713648 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. r=me with nits addressed.

::: js/src/builtin/RegExp.js
@@ +185,1 @@
>          firstDollerIndex = callFunction(std_String_indexOf, replaceValue, "$");

Because I just see it: Please fix the "Doller" typo if that doesn't already happen in another patch in the series.

@@ +344,5 @@
>      return accumulatedResult + SubstringKernel(S, nextSourcePosition | 0,
>                                                 (lengthS - nextSourcePosition) | 0);
>  }
>  
> +// Optimized path for @@replace with global flag, short string.

Please include a spec link here, too.
Attachment #8713648 - Flags: review?(till) → review+
Comment on attachment 8713649 [details] [diff] [review]
Part 24: Use RegExpSearcher in RegExp.prototype[@@search] optimized path.

Review of attachment 8713649 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: js/src/builtin/RegExp.js
@@ +145,5 @@
>          n++;
>      }
>  }
>  
> +function IsRegExpMethodOptimizable(rx) {

Please add a doc comment explaining what this tests above the function. Makes it much easier to quickly reference it when reading code that uses this function.

@@ +188,5 @@
>      // Step 7.
>      var global = !!rx.global;
>  
>      // Optimized paths for simple cases.
> +    if (!functionalReplace && firstDollerIndex === -1 && IsRegExpMethodOptimizable(rx)) {

Same as in previous patch comment: s/Doller/Dollar/
Attachment #8713649 - Flags: review?(till) → review+
Comment on attachment 8713651 [details] [diff] [review]
Part 25: Mark sunspider/check-string-unpack-code.js timeout on cgc jittest.

Review of attachment 8713651 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8713651 - Flags: review?(till) → review+
(Assignee)

Comment 141

3 years ago
Thank you for reviewing :D

(In reply to Till Schneidereit [:till] from comment #137)
> Comment on attachment 8713643 [details] [diff] [review]
> Part 20.1: Use dedicated SpeciesConstructor until bug 1165053 is fixed.
> 
> Review of attachment 8713643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I'd prefer to do this in a less hacky way. Can't we keep using the
> current version, and once we enable Symbol.species at all, we enable it for
> all builtins? Or am I overlooking something and the rest of the changes here
> somehow don't work without the species support?

@@split will work without @@species support, only with testcase fix.
I just wanted to launch @@split and RegExp[@@species] at once, as @@split is the only consumer of RegExp[@@species] in the spec.

Maybe, we can just wait for bug 1165053. as it already has patches.
(In reply to Tooru Fujisawa [:arai] from comment #141)
> @@split will work without @@species support, only with testcase fix.
> I just wanted to launch @@split and RegExp[@@species] at once, as @@split is
> the only consumer of RegExp[@@species] in the spec.
> 
> Maybe, we can just wait for bug 1165053. as it already has patches.

I think that'd make sense, yes. Do we have other uses of @@species (apart from Promise) that we wouldn't land at roughly the same time? The v8 team are landing support for well-known symbols only when they support them on all relevant builtins, so feature testing works as expected. I think that's a good approach and we should do the same where possible.
(Assignee)

Comment 143

3 years ago
We already have Symbol.species, and Map and Set already have @@species getter (bug 1131043).

TypedArray and ArrayBuffer will get @@species in bug 1165053, so they could be landed at the same time.

Array (bug 1165052) needs some more work, like self-hosting Array#concat (bug 1233642), and figure out if we can self-host Array#slice without notable perf-regression (or just decide to continue using native impl for now).
after that, I should rebase WIP patches in bug 1165052 onto them.

anyway, IMO, feature test should be done for each prototype.
(In reply to Tooru Fujisawa [:arai] from comment #143)
> We already have Symbol.species, and Map and Set already have @@species
> getter (bug 1131043).
> 
> TypedArray and ArrayBuffer will get @@species in bug 1165053, so they could
> be landed at the same time.
> 
> Array (bug 1165052) needs some more work, like self-hosting Array#concat
> (bug 1233642), and figure out if we can self-host Array#slice without
> notable perf-regression (or just decide to continue using native impl for
> now).
> after that, I should rebase WIP patches in bug 1165052 onto them.

Ok, thanks for the overview.

> anyway, IMO, feature test should be done for each prototype.

I agree, but that doesn't mean that that's how people are actually doing it :( We have little influence on that, and from what I heard people are already introducing feature tests that just check for the existence of a well-known symbol.

Comment 145

3 years ago
As the maintainer of the es5-shim and es6-shim, please please do not ever release a well-known symbol until 100% of its semantics are implemented.
(Assignee)

Comment 146

3 years ago
what do people actually do?
as we already have Symbol.species in release channel, "testing a well-know symbol" approach already doesn't work for @@species.
which bug should we fix at the same time?  and how does it help people?
Comment on attachment 8714542 [details] [diff] [review]
Part 11.1: Add testcase for String.prototype.match.

Review of attachment 8714542 [details] [diff] [review]:
-----------------------------------------------------------------

Great tests, thanks.
Attachment #8714542 - Flags: review?(till) → review+
Comment on attachment 8714545 [details] [diff] [review]
Part 13: Implement RegExp.prototype[@@search].

Review of attachment 8714545 [details] [diff] [review]:
-----------------------------------------------------------------

Just as the patch introducing the symbol, this should be folded into the patch that actually uses RegExp.prototype[@@search] in String.prototype.search. It's nice for reviewing - and for compartmentalizing the implementation work - to have it split up like this, but for landing we want it to be a bit more atomic in this case.

::: js/src/builtin/RegExp.js
@@ +142,5 @@
>          n++;
>      }
>  }
>  
> +// ES 2016 draft Dec 10, 2015 21.2.5.9.

Please update this to the current draft. ( It was already outdated when you asked for review - I wouldn't ask you to do it if it just became an issue because I let you wait for the review for too long ;) )

@@ +144,5 @@
>  }
>  
> +// ES 2016 draft Dec 10, 2015 21.2.5.9.
> +function RegExpSearch(string) {
> +    // Steps 1-2.

Split these up into two comments. The `Steps n-m` thing should only be used if the steps are a single expression (or block).
Attachment #8714545 - Flags: review?(till) → review+
Comment on attachment 8714548 [details] [diff] [review]
Part 14: Call RegExp.prototype[@@search] from String.prototype.search.

Review of attachment 8714548 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: js/src/builtin/String.js
@@ +83,5 @@
> +}
> +
> +function IsStringSearchOptimizable() {
> +    var RegExpProto = GetBuiltinPrototype("RegExp");
> +    if (!RegExpPrototypeOptimizable(RegExpProto))

I'd fold all this into
// If RegExpPrototypeOptimizable succeeds, `exec` and `@@search` are
// guaranteed to be data properties.
return RegExpPrototypeOptimizable(RegExpProto) &&
       RegExpProto.exec === RegExp_prototype_Exec &&
       RegExpProto[std_search] === RegExpSearch;

@@ +120,5 @@
> +        flags = arguments[1];
> +        WarnOnceAboutFlagsArgument();
> +    } else {
> +        if (IsString(regexp) && IsStringSearchOptimizable()) {
> +            var flatResult = FlatStringSearch(string, regexp);

Would it make sense to do the IsString and IsStringSearchOptimizable checks inside FlatStringSearch? The IsString check actually happens in step 2 above, so you could also extract it there and reuse the result here. Yes, Ion will probably do all that, but baseline won't.

At the very least, FlatStringSearch should assert that these preconditions hold, though.
Attachment #8714548 - Flags: review?(till) → review+
(Assignee)

Updated

3 years ago
See Also: → 1249235
(Assignee)

Comment 150

3 years ago
rebasing, renumbering, folding, addressing review comments, updating ES 2016 spec steps, etc.
Attachment #8714718 - Attachment is obsolete: true
Attachment #8721627 - Flags: review+
(Assignee)

Comment 151

3 years ago
Attachment #8713618 - Attachment is obsolete: true
Attachment #8721628 - Flags: review+
(Assignee)

Comment 152

3 years ago
Attachment #8714901 - Attachment is obsolete: true
Attachment #8721629 - Flags: review+
(Assignee)

Comment 153

3 years ago
Attachment #8713621 - Attachment is obsolete: true
Attachment #8721630 - Flags: review+
(Assignee)

Comment 154

3 years ago
Attachment #8713622 - Attachment is obsolete: true
Attachment #8721631 - Flags: review+
(Assignee)

Comment 155

3 years ago
Attachment #8713623 - Attachment is obsolete: true
Attachment #8721632 - Flags: review+
(Assignee)

Comment 156

3 years ago
Attachment #8713624 - Attachment is obsolete: true
Attachment #8721633 - Flags: review+
(Assignee)

Comment 157

3 years ago
Attachment #8714029 - Attachment is obsolete: true
Attachment #8721634 - Flags: review+
(Assignee)

Comment 159

3 years ago
Attachment #8716891 - Attachment is obsolete: true
Attachment #8716891 - Flags: review?(till)
Attachment #8721636 - Flags: review?(till)
(Assignee)

Comment 160

3 years ago
Attachment #8713629 - Attachment is obsolete: true
Attachment #8714545 - Attachment is obsolete: true
Attachment #8714548 - Attachment is obsolete: true
Attachment #8721637 - Flags: review+
(Assignee)

Comment 161

3 years ago
Attachment #8716892 - Attachment is obsolete: true
Attachment #8716892 - Flags: review?(till)
Attachment #8721638 - Flags: review?(till)
(Assignee)

Comment 162

3 years ago
Attachment #8713632 - Attachment is obsolete: true
Attachment #8721639 - Flags: review+
(Assignee)

Comment 163

3 years ago
Attachment #8714549 - Attachment is obsolete: true
Attachment #8714549 - Flags: review?(till)
Attachment #8721640 - Flags: review?(till)
(Assignee)

Comment 165

3 years ago
Attachment #8716893 - Attachment is obsolete: true
Attachment #8716893 - Flags: review?(till)
Attachment #8721642 - Flags: review?(till)
(Assignee)

Comment 166

3 years ago
Attachment #8713637 - Attachment is obsolete: true
Attachment #8721643 - Flags: review+
(Assignee)

Comment 167

3 years ago
Attachment #8713638 - Attachment is obsolete: true
Attachment #8721644 - Flags: review+
(Assignee)

Comment 168

3 years ago
Attachment #8714553 - Attachment is obsolete: true
Attachment #8714553 - Flags: review?(till)
Attachment #8721645 - Flags: review?(till)
(Assignee)

Comment 169

3 years ago
Attachment #8716200 - Attachment is obsolete: true
Attachment #8716200 - Flags: review?(till)
Attachment #8721646 - Flags: review?(till)
(Assignee)

Comment 170

3 years ago
Attachment #8713647 - Attachment is obsolete: true
Attachment #8721647 - Flags: review+
(Assignee)

Comment 173

3 years ago
Attachment #8713643 - Attachment is obsolete: true
Attachment #8713651 - Attachment is obsolete: true
Attachment #8714542 - Attachment is obsolete: true
Attachment #8716201 - Attachment is obsolete: true
Attachment #8721650 - Flags: review+
Comment on attachment 8721636 [details] [diff] [review]
Part 9.1: Reflect bug 1245801 patch for match.

Review of attachment 8721636 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8721636 - Flags: review?(till) → review+
Comment on attachment 8721638 [details] [diff] [review]
Part 10.1: Reflect bug 1245801 patch for search.

Review of attachment 8721638 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8721638 - Flags: review?(till) → review+
Comment on attachment 8721640 [details] [diff] [review]
Part 11.1: Implement RegExp.prototype[@@replace].

Review of attachment 8721640 [details] [diff] [review]:
-----------------------------------------------------------------

As many times before: this is very impressive. r=me with feedback addressed.

::: js/src/builtin/RegExp.cpp
@@ +1081,5 @@
>      return status != RegExpRunStatus_Error;
>  }
>  
> +static void
> +getParen(JSLinearString* matched, AutoValueVector& captures, size_t index, JSSubString* out)

I think it'd be better to just pass the value here and do the indexing operation in the callers.

@@ +1089,5 @@
> +        out->initEmpty(matched);
> +        return;
> +    }
> +    MOZ_ASSERT(capture.isString());
> +    MOZ_ASSERT(capture.toString()->isLinear());

Nit: redundant asserts.

@@ +1090,5 @@
> +        return;
> +    }
> +    MOZ_ASSERT(capture.isString());
> +    MOZ_ASSERT(capture.toString()->isLinear());
> +    JSLinearString* captureLinear = capture.toString()->ensureLinear(nullptr);

use ->asLinear() here.

@@ +1104,5 @@
> +                JSSubString* out, size_t* skip)
> +{
> +    MOZ_ASSERT(*dp == '$');
> +
> +    /* If there is only a dollar, bail now */

nit: "." at the end of full-sentence comments.

@@ +1131,5 @@
> +            }
> +        }
> +        if (num == 0) {
> +            // The result is implementation-defined.
> +            // Do not substitute.

Nit: make this one line and separated by a "," not a "."

@@ +1139,5 @@
> +        *skip = cp - dp;
> +
> +        MOZ_ASSERT(num <= captures.length());
> +
> +        getParen(matched, captures, num - 1, out);

As said above, this should be
getParen(matched, captures[num - 1], out);

@@ +1145,5 @@
> +    }
> +
> +    *skip = 2;
> +    switch (dc) {
> +      case '$':

If you add a `default: return false` (ideally before the first case), you can replace all the `return true`s in the cases with a single one at the end of the function.

@@ +1177,5 @@
> +    JS::AutoCheckCannotGC nogc;
> +    MOZ_ASSERT(firstDollarIndex < replacement->length());
> +    const CharT* bp = replacement->chars<CharT>(nogc);
> +    const CharT* dp = bp + firstDollarIndex;
> +    const CharT* ep = bp + replacement->length();

Please give all these locals self-explanatory names. In some cases long names can be a hindrance to understanding code, but this is not one of them.

@@ +1230,5 @@
> +          HandleLinearString replacement, size_t firstDollarIndex, StringBuffer &sb)
> +{
> +    JS::AutoCheckCannotGC nogc;
> +    const CharT* bp = replacement->chars<CharT>(nogc);
> +    const CharT* cp = bp;

As above: please give the locals self-explanatory names.

@@ +1260,5 @@
> +}
> +
> +/* ES 2016 draft Feb 17, 2016 21.1.3.14.1. */
> +bool
> +js::GetSubstitution(JSContext* cx, HandleLinearString matched, HandleLinearString string,

Please name this RegExpGetSubstitution. I know the spec calls it this, but we should namespace it in our implementation because it's really not self-explanatory that this is RegExp-related.

::: js/src/builtin/RegExp.h
@@ +106,5 @@
>  extern bool
>  RegExpInstanceOptimizableRaw(JSContext* cx, JSObject* rx, JSObject* proto, uint8_t* result);
>  
> +extern bool
> +GetSubstitution(JSContext* cx, HandleLinearString matched, HandleLinearString string,

Please rename this to RegExpGetSubstitution.

@@ +108,5 @@
>  
> +extern bool
> +GetSubstitution(JSContext* cx, HandleLinearString matched, HandleLinearString string,
> +                size_t position, HandleObject capturesObj, HandleLinearString replacement,
> +                size_t firstDollerIndex, MutableHandleValue rval);

s/Doller/Dollar/

::: js/src/builtin/RegExp.js
@@ +149,5 @@
>  
> +function IsRegExpReplaceOptimizable(rx) {
> +    var RegExpProto = GetBuiltinPrototype("RegExp");
> +    // If RegExpPrototypeOptimizable and RegExpInstanceOptimizable succeed,
> +    // `exec` is guaranteed to be data properties.

"`RegExpProto.exec` is guaranteed to be a data property."

@@ +202,5 @@
> +        rx.lastIndex = 0;
> +    }
> +
> +    // Step 9.
> +    var results = [];

This is specified to be a list, so all write accesses below have to be _DefineDataProperty calls. Please add a test that would've caught this.

@@ +256,5 @@
> +
> +        var n, capN, replacement;
> +        if (functionalReplace || firstDollarIndex !== -1) {
> +            // Step 14.h.
> +            var captures = [];

Same as for results: writing elements mustn't be content-observable (or disruptable), so neither [[Set]], nor std_Array_push can be used. Please add a test.

@@ +281,5 @@
> +            }
> +
> +            // Step 14.j.
> +            if (functionalReplace) {
> +                if (nCaptures == 0) {

Please change this to be a switch statement instead of if..else if..else blocks.

@@ +336,5 @@
> +    if (nextSourcePosition >= lengthS)
> +        return accumulatedResult;
> +
> +    // Step 16.
> +    return accumulatedResult + SubstringKernel(S, nextSourcePosition | 0,

These coercions are just for performance, right? Ideally you'd apply them during computation of the local, e.g. in step 14.1.iii. That way they can be applied everywhere.

@@ +381,5 @@
> +        var position = result.index;
> +        lastIndex = position + matchLength;
> +
> +        // Step 14.l.ii.
> +        accumulatedResult += SubstringKernel(S, nextSourcePosition | 0,

Same comment about the coercions as above.

@@ +432,5 @@
> +    // Steps 14.e-f.
> +    var position = result.index;
> +
> +    // Step 14.l.ii.
> +    var accumulatedResult = SubstringKernel(S, 0, position | 0) + replaceValue;

And here.

::: js/src/tests/ecma_6/RegExp/replace-this.js
@@ +1,2 @@
> +var BUGNUMBER = 887016;
> +var summary = "RegExp.prototype[@@replace] should check this value.";

s/this/|this|/

::: js/src/vm/SelfHosting.cpp
@@ +1364,5 @@
>      return RegExpCreate(cx, args[0], args[1], args.rval());
>  }
>  
> +static bool
> +intrinsic_GetSubstitution(JSContext* cx, unsigned argc, Value* vp)

Please name this something like "RegExpGetSubstitution". Here and for the JS-reflected name.

@@ +1370,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    MOZ_ASSERT(args.length() == 6);
> +    MOZ_ASSERT(args[0].isString());
> +    MOZ_ASSERT(args[1].isString());

All these |isFoo| asserts are redundant with the checks that happen in the |asFoo| coercions below.

@@ +1393,5 @@
> +#endif
> +
> +    RootedString replacement(cx, args[4].toString());
> +
> +    int32_t firstDollerIndex;

s/Doller/Dollar/
Please run a grep over all patches to ensure you catch all instances of this.

@@ +1394,5 @@
> +
> +    RootedString replacement(cx, args[4].toString());
> +
> +    int32_t firstDollerIndex;
> +    if (!ToInt32(cx, args[5], &firstDollerIndex))

Just using toNumber and casting the result to int32_t should be fine here.
Attachment #8721640 - Flags: review?(till) → review+
Comment on attachment 8721641 [details] [diff] [review]
Part 11.2: Call RegExp.prototype[@@replace] from String.prototype.replace. r=h4writer

Review of attachment 8721641 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, the diff stats for this are amazing.
r=me with feedback addressed.

::: js/src/builtin/String.js
@@ +70,5 @@
>          ThrowTypeError(JSMSG_MISSING_FUN_ARG, 0, 'String.match');
>      return callFunction(String_match, thisValue, regexp);
>  }
>  
> +function StringHasNoReplace() {

Please call this StringProtoHasNoReplace. Otherwise it sounds like this is talking about a String instance or a string.

@@ +139,5 @@
> +    var newString;
> +    if (pos === 0)
> +        newString = "";
> +    else
> +        newString = SubstringKernel(string, 0 | 0, pos | 0);

0|0 is definitely not required. (Well, if it is, please file a bug against the JITs.)
The `pos` coercion should, as in other patches, be done when computing `pos`, so the optimization is applied throughout. Same for the other optimization coercions below.

::: js/src/jit-test/tests/ion/bug977966.js
@@ +115,5 @@
>      // SpiderMonkey extension
>      assertEq(s2.replace("", "" + i, "g") , i + "a" + i + "b" + i + "c" + i);
>  }
>  
> +for (var i = 0; i < 1000; ++i) {

Can't we instead run this test with ion-eager? Especially in debug builds this will otherwise needlessly slow down test runs.

::: js/src/vm/RegExpObject.h
@@ +533,5 @@
>  
>  extern JSObject*
>  CloneScriptRegExpObject(JSContext* cx, RegExpObject& re);
>  
> +/* Escape all slash and newline in given string. */

"slashes and newlines in the given"

::: js/src/vm/SelfHosting.cpp
@@ +1420,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 3);
> +    MOZ_ASSERT(args[0].isString());
> +    MOZ_ASSERT(args[1].isString());
> +    MOZ_ASSERT(args[2].isString());

Nit: redundant asserts.

@@ +1434,5 @@
> +    return true;
> +}
> +
> +static bool
> +intrinsic_EscapeRegExpMetaChars(JSContext* cx, unsigned argc, Value* vp)

Please call this RegExpEscapeMetaChars. Here and below.

@@ +1439,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    MOZ_ASSERT(args.length() == 1);
> +    MOZ_ASSERT(args[0].isString());

Same here.
Attachment #8721641 - Flags: review?(till) → review+
Comment on attachment 8721642 [details] [diff] [review]
Part 11.3: Reflect bug 1245801 patch for replace.

Review of attachment 8721642 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8721642 - Flags: review?(till) → review+
Comment on attachment 8721645 [details] [diff] [review]
Part 13.1: Implement RegExp.prototype[@@split].

Review of attachment 8721645 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice. r=me with feedback addressed.

::: js/src/builtin/RegExp.js
@@ +488,5 @@
> +        return false;
> +
> +    var RegExpProto = RegExpCtor.prototype;
> +    // If RegExpPrototypeOptimizable succeeds, `exec` is guaranteed to be data
> +    // properties.

"`RegExpProto.exec` is guaranteed to be a data property."

@@ +557,5 @@
> +        if (z !== null)
> +            return A;
> +
> +        // Step 17.d.
> +        A[0] = S;

This and the other writes to `A` need to use _DefineDataProperty. It's somewhat surprising that the spec has us directly create an Array instead of a List, but the data writes are specified in a way that makes them unobservable.
Please add a test that would've caught this.
Attachment #8721645 - Flags: review?(till) → review+
Comment on attachment 8721646 [details] [diff] [review]
Part 13.2: Call RegExp.prototype[@@split] from String.prototype.split. r=h4writer

Review of attachment 8721646 [details] [diff] [review]:
-----------------------------------------------------------------

Again, very nice.
r=me with feedback addressed.


I'm truly sorry it took me so long to finish these reviews. This is exceptional work and would've deserved quicker turnaround from me.

::: js/src/builtin/String.js
@@ +221,5 @@
>          ThrowTypeError(JSMSG_MISSING_FUN_ARG, 0, 'String.search');
>      return callFunction(String_search, thisValue, regexp);
>  }
>  
> +function StringHasNoSplit() {

As for Replace: please call this StringProtoHasNoSplit.

::: js/src/jsstr.cpp
@@ +2377,3 @@
>      size_t lastEndIndex = 0;
> +
> +    // Steps 13.

nit: "Step"

::: js/src/vm/SelfHosting.cpp
@@ +1457,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    MOZ_ASSERT(args.length() == 2);
> +    MOZ_ASSERT(args[0].isString());
> +    MOZ_ASSERT(args[1].isString());

nit: redundant asserts.

@@ +1483,5 @@
> +
> +    MOZ_ASSERT(args.length() == 3);
> +    MOZ_ASSERT(args[0].isString());
> +    MOZ_ASSERT(args[1].isString());
> +    MOZ_ASSERT(args[2].isNumber());

Same here.

@@ +1492,5 @@
> +
> +    // args[2] should be already in UInt32 range, but it could be double typed,
> +    // because of Ion optimization.
> +    uint32_t limit;
> +    MOZ_ALWAYS_TRUE(ToUint32(cx, args[2], &limit));

make this use toNumber() + a simple cast.
Attachment #8721646 - Flags: review?(till) → review+
(Assignee)

Comment 181

3 years ago
Thank you for reviewing :D

(In reply to Till Schneidereit [:till] from comment #176)
> @@ +336,5 @@
> > +    if (nextSourcePosition >= lengthS)
> > +        return accumulatedResult;
> > +
> > +    // Step 16.
> > +    return accumulatedResult + SubstringKernel(S, nextSourcePosition | 0,
> 
> These coercions are just for performance, right? Ideally you'd apply them
> during computation of the local, e.g. in step 14.1.iii. That way they can be
> applied everywhere.

These coercions are because of the restriction of the SubstringKernel function that accepts only int32-typed values.

https://dxr.mozilla.org/mozilla-central/rev/46210f3ae07855edfe1383210d78433e38a6b564/js/src/builtin/String.js#8
> function String_substring(start, end) {
> ...
>     // While |from| and |to - from| are bounded to the length of |str| and this
>     // and thus definitely in the int32 range, they can still be typed as
>     // double. Eagerly truncate since SubstringKernel only accepts int32.
>     return SubstringKernel(str, from | 0, (to - from) | 0);

I'll check if coercing at each assignment works or not.
(Assignee)

Comment 182

3 years ago
mmm, keeping the value int32-typed at the assignment seems to be error prone and not-robust for further change, and doesn't make sense in term of spec steps.
I'll add a thin wrapper for SubstringKernel for the substring operation used in the spec. that coerces the arguments into int32, but doesn't check the type/range like String_substr.
(to be clear, I was about to use String_substr/String_slice, but type/range checks in them were redundant)
(Assignee)

Comment 183

3 years ago
Added Substring function and changed SubstringKernel consumers added in Part 11 (replace) and 12 (split) to call it instead.

Arguments passed there is guaranteed to be int32 range, and also in valid range in the string, but Ion could use double type for them.
Attachment #8727228 - Flags: review?(till)
Comment on attachment 8727228 [details] [diff] [review]
Part 11 and 12 followup: Add a thin wrapper function for SubstringKernel to coerce arguments into int32.

Review of attachment 8727228 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that's much better. I agree that the previous solution was somewhat brittle.
Attachment #8727228 - Flags: review?(till) → review+
(Assignee)

Comment 185

3 years ago
as noted in bug 1165053,  I'll push these patches at the same time as bug 1165052 (Array[@@species]) and bug 1165053 (%TypedArray%[@@species] and ArrayBuffer[@@species]).
(Assignee)

Comment 186

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/10621b5e7de50d4df6190354e334309a7b6987d9
Bug 887016 - Part 1: Add native RegExpCreate. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/adca5076bc4605316984c719bbc4729e0a7f1d58
Bug 887016 - Part 2: Add self-hosting RegExpCreate intrinsic. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/043f06dd9135a5ce9e716f12805d4df686da2ebc
Bug 887016 - Part 3: Add LookupOwnPropertyPure. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/67346678966fc8c40546ab49521b43049fcc851f
Bug 887016 - Part 4: Add GetOwnNativeGetterPure. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/b77cb94666ca383ff7586997aa820b692794f557
Bug 887016 - Part 5: Add HasOwnDataPropertyPure. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e25768c99addacea51258b2392926749da2df5
Bug 887016 - Part 6: Add RegExpPrototypeOptimizable. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/dee348be668513677d339437e9fcf4d6b5b6ce01
Bug 887016 - Part 7: Add RegExpInstanceOptimizable. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/4be734a1452428151831dbbc6115ac10b20083f7
Bug 887016 - Part 8: Add ObjectHasPrototype. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/537d40121b6d4c80071e02aeaa95712fdfdcb107
Bug 887016 - Part 9: Implement RegExp.prototype[@@match] and call it from String.prototype.match. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/f23a61067cefec53fd72dc00383092f72fe707cb
Bug 887016 - Part 10: Implement RegExp.prototype[@@search] and call it from String.prototype.search. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/f373233a8c82941b6ca1dfca5fd62edb3ceae81a
Bug 887016 - Part 11: Implement RegExp.prototype[@@replace] and call it from String.prototype.replace. r=h4writer,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/716a5a6539d73d8d3fb70a354333b713beb1acdb
Bug 887016 - Part 12: Implement RegExp[@@species] getter. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/01da4d30fd114f532009a77acfc663fd54699502
Bug 887016 - Part 13: Implement RegExp.prototype[@@split] and call it from String.prototype.split. r=h4writer,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/7db6a99ec5462372a107b3e346aa8c1ecffaa4ae
Bug 887016 - Part 14: Add RegExpSearcher. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/817e8aab2348ecd7a16df359e054ef8fb14491de
Bug 887016 - Part 15: Use RegExpSearcher in RegExp.prototype[@@replace] optimized path. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/58946916593bf635aad899497442fcfd884e97c0
Bug 887016 - Part 16: Use RegExpSearcher in RegExp.prototype[@@search] optimized path. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/b903c18bbb32c68c210a45da69ee3cbbe2ebeb41
Bug 887016 - Part 17: Mark sunspider/check-string-unpack-code.js timeout on cgc jittest. r=till
(Assignee)

Comment 187

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5df43f8950ad898668b16f36fa410f80d126a2
Backed out changeset b903c18bbb32 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7596439fda73345a5eecddb205c2508967a8eaa7
Backed out changeset 58946916593b (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb8165a2a66701fc5550b19ba29b3fa3ffd4714
Backed out changeset 817e8aab2348 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d44ccce05064d63cd96efd511694bc1a28d94646
Backed out changeset 7db6a99ec546 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/5676c7b622c71064d62f8e3a57c7d35d1043f9bc
Backed out changeset 01da4d30fd11 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bfefec1be82ae534a69701d0e99ab664ea209bb
Backed out changeset 716a5a6539d7 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/185994606889363e10d446e0fb8835fab9fa19f2
Backed out changeset f373233a8c82 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fd465ec1e2c279f7ae3d9c4243c1d6c2f597c43
Backed out changeset f23a61067cef (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/248bb4773adfc0c076922468f938f8567076b528
Backed out changeset 537d40121b6d (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/592fbf849342b06d080078bc654f119b023c6b58
Backed out changeset 4be734a14524 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd7e704523d7138f12db889e9d92c6000ba88993
Backed out changeset dee348be6685 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/2081fb1b83a1dc8f1ba34fcf11bca7e621e20b9d
Backed out changeset 56e25768c99a (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecba6c9f0bc38ece3305da035d43ffedd111bbab
Backed out changeset b77cb94666ca (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd34be112501af92dcaf4f4f24e7f67a7ceaaf7e
Backed out changeset 67346678966f (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5060d63b4f8ddbb8a5f9e6db822e283701f46f0
Backed out changeset 043f06dd9135 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e34eeaf571fc541c0dda5198bffcc3cea6b3977
Backed out changeset adca5076bc46 (bug 887016)

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf8ca358a383f7a119960155f0532c542157636
Backed out changeset 10621b5e7de5 (bug 887016)
(Assignee)

Comment 188

3 years ago
jsreftest harness doesn't like modified Array.prototype, and we should remove all fake setters after all tests done, both in replace-trace.js and split-trace.js tests.
Attachment #8736208 - Flags: review?(till)
(Assignee)

Comment 190

3 years ago
the list of constructor properties for RegExp seems to have to be updated to include Symbol.species (as Array's case is fixed by adding Symbol.species), but I face the issue that @@species is not enumerated for RegExp.
can you give me some advice?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9908b6e2a1&selectedJob=18765596
Attachment #8736213 - Flags: review?(bobbyholley)
(Assignee)

Comment 191

3 years ago
(In reply to Tooru Fujisawa [:arai] from comment #190)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3c9908b6e2a1&selectedJob=18765596

sorry I posted wrong try run (the result is same tho)

I meant following one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=050194f1a44b&selectedJob=18765508
> 2754 INFO TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xul | getOwnPropertySymbols works on Xrayed ctors - got "[]", expected "[\"Symbol.species\"]"
(Assignee)

Comment 192

3 years ago
sorry, maybe I found the reason (or some issue related to it),

ShouldResolveStaticProperties return false for RegExp, because of other static properties, and RegExp ctor's props are not resolved, including species.
https://dxr.mozilla.org/mozilla-central/rev/d5d53a3b4e50b94cdf85d20690526e5a00d5b63e/js/xpconnect/wrappers/XrayWrapper.cpp#478

> static bool
> ShouldResolveStaticProperties(JSProtoKey key)
> {
>     // Don't try to resolve static properties on RegExp, because they
>     // have issues.  In particular, some of them grab state off the
>     // global of the RegExp constructor that describes the last regexp
>     // evaluation in that global, which is not a useful thing to do
>     // over Xrays.
>     return key != JSProto_RegExp;
> }

we might have to resolve only @@species, or only Symbols?
(Assignee)

Comment 193

3 years ago
Changed the ShouldResolveStaticProperties and its consumers to following:
  * Renamed ShouldResolveStaticProperties to ShouldResolve,
    and added ResolveTargetKind parameter that is Constructor or Prototype,
    and added jsid parameter that is the id of the property
  * ShouldResolve returns false for RegExp constructor non-symbol properties (that is, non-standard static props)
  * While resolving, if ShouldResolve returns false (means, it's a non-standard RegExp static props), it doesn't call TryResolvePropertyFromSpecs
  * While enumerating, it doesn't enumerate if ShouldResolve returns false

With these changes, only RegExp[@@species] is affected and it's enumerated/resolved in xray.

whole functions/types could be made more RegExp specific, but I have no other better idea.
Attachment #8736604 - Flags: review?(bobbyholley)
Attachment #8736213 - Flags: review?(bobbyholley) → review+
Comment on attachment 8736604 [details] [diff] [review]
Part 12.2: Resolve RegExp constructor symbol props in xray.

Review of attachment 8736604 [details] [diff] [review]:
-----------------------------------------------------------------

Can you explain the motivation for supporting RegExp[@@species] over Xrays? The reason Xrays currently don't resolve static properties on Xrays is that some of the properties are tricky (since they access global state), and I couldn't think of a good use-case where somebody would want to use RegExp over Xrays (or in another global at all, really).

This patch looks sane, but it seems odd to do this work to specifically support @@species while ignoring all the other properties, unless there's some specific reason to do it that I'm missing.
(Assignee)

Comment 195

3 years ago
Thank you for reviewing :)

Actually, I just thought I should support the property over Xrays if it's a standard property.
Do you think that it's okay to just add Symbol.species to ctorPropsToSkip for RegExp in js/xpconnect/tests/chrome/test_xrayToJS.xul if the use case won't be important?
In that case, I'll prepare a patch to just skip it.


Currently, RegExp[@@species] is used only in RegExp#split, while copying |this| RegExp with adding sticky flag.
  https://tc39.github.io/ecma262/#sec-regexp.prototype-@@split (steps 4 and 10)

Then, if we don't support RegExp[@@species] over Xrays, step 5 in SpeciesConstructor will return undefined, and defaultConstructor (%RegExp% in calling global) will be used.
  https://tc39.github.io/ecma262/#sec-speciesconstructor

Then, the copied RegExp object will be used for calling RegExpExec (that would calls .exec, .global, and .sticky),

so, supporting/not-supporting RegExp[@@species] would affect only if the RegExp prototype's exec, global, and sticky properties is modified in the |this| RegExp object's global.  I don't think this case is so much important, unless the author of the script tries to use the global to just get a dedicated RegExp prototype for modifying the built-in properties (I thought there was such use case for, maybe Array tho)
Comment on attachment 8736604 [details] [diff] [review]
Part 12.2: Resolve RegExp constructor symbol props in xray.

Review of attachment 8736604 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah. Realistically our RegExp Xray story isn't great, so I'd rather just not support complicated stuff for now. If we find a compelling use-case, there's other stuff we'll need to fix here too.
Attachment #8736604 - Flags: review?(bobbyholley) → review-
Comment on attachment 8736208 [details] [diff] [review]
Part 11.1: Remove Array.prototype getter props after test to avoid jsreftest bustage.

Review of attachment 8736208 [details] [diff] [review]:
-----------------------------------------------------------------

Bah, it'd be much better if our harness wasn't as easily broken. Perhaps by running all tests in a throwaway global. But anyway, this is good as a fix, but please merge it into the patch that'd break the tests otherwise.
Attachment #8736208 - Flags: review?(till) → review+
Comment on attachment 8736210 [details] [diff] [review]
Part 13.1: Remove Array.prototype getter props after test to avoid jsreftest bustage.

Review of attachment 8736210 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, please also merge this into the breaking patch.
Attachment #8736210 - Flags: review?(till) → review+
(Assignee)

Comment 199

3 years ago
test_xrayToJS.xul may need some more refactoring to support symbols, I'd leave it to other bug for now.
Attachment #8736604 - Attachment is obsolete: true
Attachment #8737727 - Flags: review?(bobbyholley)
(Assignee)

Comment 200

3 years ago
ion/bug977966.js hits timeout on cgc, intermittently, "--ion-eager --ion-offthread-compile=off" options.
this is a test for split-join optimization, and I don't see any difference in the time taken by the test on non-cgc run.
So, just added it to timeout list.

  https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/bug977966.js
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2c283aee9db&selectedJob=18838818
Attachment #8737986 - Flags: review?(till)
(Assignee)

Comment 201

3 years ago
js/src/jit-test/tests/basic/bug754150.js is hitting timeout on linux32 debug jit test.
the test does gczeal(4) and calls String#replace, and it takes too long time if replace is self-hosted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f6fc8a69e6e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=19010368


I don't see any much perf regression if I remove gczeal there, so I believe it's just an issue between gczeal and self-hosted function, mostly in interpreter mode, and we could quit this test if jit is disabled.

before   no options              with gczeal    :  0.741s
before   no options              without gczeal :  0.277s
before   --no-baseline --no-ion  with gczeal:   :  1.125s
before   --no-baseline --no-ion  without gczeal :  0.264s
after    no options              with gczeal    :  5.782s
after    no options              without gczeal :  0.290s
after    --no-baseline --no-ion  with gczeal:   : 25.218s
after    --no-baseline --no-ion  without gczeal :  0.284s
Attachment #8738059 - Flags: review?(till)
Comment on attachment 8738059 [details] [diff] [review]
Part 17.2: Do not run jit-test basic/bug754150.js with --no-baseline.

Review of attachment 8738059 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense to me. inJit is pretty wonky, but oh well.

::: js/src/jit-test/tests/basic/bug754150.js
@@ +1,5 @@
>  // |jit-test| error: TypeError;
> +
> +// String.prototype.replace takes too long time with gczeal(4) if
> +// --no-baseline --no-ion options.
> +if (typeof inJit == "function" && typeof inJit() == "string")

Can you add an assertEq(inJit(), "Baseline is disabled.") here so this doesn't do unexpected things if the behavior of inJit is ever changed?
Attachment #8738059 - Flags: review?(till) → review+
Comment on attachment 8737986 [details] [diff] [review]
Part 17.1: Mark ion/bug977966.js timeout on cgc jittest.

Review of attachment 8737986 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8737986 - Flags: review?(till) → review+
Comment on attachment 8737727 [details] [diff] [review]
Part 12.2: Skip RegExp[@@species] in xray test.

Review of attachment 8737727 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. Sorry this got lost. :-(

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +416,5 @@
>         ctorProps.toSource(), "getOwnPropertyNames works on Xrayed ctors");
>      is(Object.getOwnPropertySymbols(xrayCtor).map(uneval).sort().toSource(),
> +       gConstructorProperties[classname]
> +       .filter(id => typeof id !== "string" && !ctorPropsToSkip.includes(id))
> +       .map(uneval).sort().toSource(),

Can you do something like:

let ctorSymbols = filterOut(...)

to make this look more like the other code?
Attachment #8737727 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 205

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/59426007bb575d8e46a3322b14117bb716bd51f4
Bug 887016 - Part 1: Add native RegExpCreate. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/e88d83bf05bd4f3269286d4de9f15ad5c631748d
Bug 887016 - Part 2: Add self-hosting RegExpCreate intrinsic. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f92a34bcee3013dab2ab4f6961e568a9561e2fa
Bug 887016 - Part 3: Add LookupOwnPropertyPure. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db4e936d05ba3f5163cf93e6710c959d81a9abb
Bug 887016 - Part 4: Add GetOwnNativeGetterPure. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/67d158e6e363710562aca1acdfda70a9c4eb4d59
Bug 887016 - Part 5: Add HasOwnDataPropertyPure. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e69c58a1efd048b7ff235d5f5c0af24d4ab2e27
Bug 887016 - Part 6: Add RegExpPrototypeOptimizable. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/741c4be20f025d1b24b5fcab5b7987bd649bdd0b
Bug 887016 - Part 7: Add RegExpInstanceOptimizable. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac74c5fe92172d8dbb363eb713e1512570c51af1
Bug 887016 - Part 8: Add ObjectHasPrototype. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b06959919ad3a0150c7ca1dfe0de7d2d9df7e1
Bug 887016 - Part 9: Implement RegExp.prototype[@@match] and call it from String.prototype.match. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a37c4b0e33804f0af4e3d05fdcd087e6b359f00
Bug 887016 - Part 10: Implement RegExp.prototype[@@search] and call it from String.prototype.search. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e0ea1a1ed23ef4e9ca8e1fbdf3f8fcef1242c5
Bug 887016 - Part 11: Implement RegExp.prototype[@@replace] and call it from String.prototype.replace. r=h4writer,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd13c095d3764559d2eb23d380ef5a72a6fbfc06
Bug 887016 - Part 12: Implement RegExp[@@species] getter. r=evilpie,bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3a6133271c6072773e399eac66426ddcd3bfaf
Bug 887016 - Part 13: Implement RegExp.prototype[@@split] and call it from String.prototype.split. r=h4writer,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e25cbe3dcbcf4018b59505816de535a0c29a07
Bug 887016 - Part 14: Add RegExpSearcher. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d08e4b452331d9d8d7fd4b827217bb95fc9331
Bug 887016 - Part 15: Use RegExpSearcher in RegExp.prototype[@@replace] optimized path. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/829fee28b3ea0dc0b2d81b60e693e510811253dd
Bug 887016 - Part 16: Use RegExpSearcher in RegExp.prototype[@@search] optimized path. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0f975a23119a61a6c8e5856125de2db5713c49
Bug 887016 - Part 17: Mark sunspider/check-string-unpack-code.js timeout on cgc jittest. r=till
Depends on: 1262967
Depends on: 1263118
Depends on: 1263341
(Assignee)

Updated

3 years ago
See Also: → 1263490
Depends on: 1263525
Depends on: 1263532
(Assignee)

Updated

3 years ago
Depends on: 1263549
Depends on: 1263558
(Assignee)

Updated

3 years ago
No longer depends on: 1263525
Depends on: 1263857
(Assignee)

Updated

3 years ago
Depends on: 1263851
(Assignee)

Updated

3 years ago
Depends on: 1263879
Depends on: 1264264
Depends on: 1268034
Depends on: 1268138
Depends on: 1271037
Depends on: 1269719
(Assignee)

Updated

3 years ago
Depends on: 1287521
(Assignee)

Updated

3 years ago
Depends on: 1287524
(Assignee)

Updated

3 years ago
Depends on: 1287525
(Assignee)

Updated

3 years ago
Depends on: 1290506
(Assignee)

Updated

3 years ago
Depends on: 1290655
Depends on: 1308743
Depends on: 1314401
(Assignee)

Updated

2 years ago
Depends on: 1368732
(Assignee)

Updated

5 months ago
Depends on: 1509768
You need to log in before you can comment on or make changes to this bug.