1066040 - IonMonkey: Implement StringReplace recover instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Implement StringReplace in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
See Bug 1050649 for a similar instruction.

Comment 1

[Anuj Agarwal [:anujagarwal464]]

@nbp: I would like work on this bug. Can you give me few pointers?

Comment 2

[Nicolas B. Pierron [:nbp]]

Hi Anuj,

I think you are already working on Bug 1060938.

Otherwise you will find a lot of pointer specific to recover instruction in the links of comment 0, and you probably want to have a look at str_replace_string_raw. [1]

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.h#398

Comment 3

[Ekta Goel]

@nbp Hello, I am a newbie. I would like to try fix this bug. Can you please guide me through all the things required.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Ekta Goel from comment #3)
> @nbp Hello, I am a newbie. I would like to try fix this bug. Can you please
> guide me through all the things required.

Is there anything specific where I can help you in addition to the two links provided in the description (comment 0) of this bug?

Comment 5

[andy.zsshen]

Hi, I am new to the SpiderMonkey engine.

After some study for the comment #0 and the document, I already wrote the bailout code for StringReplace operation. Besides, for the debug build testing (~/mozilla-central/js/src/jit-test/jit_test.py ~/mozilla-central/js/src/build-debug/dist/bin/js), all the cases are passed.

But I am not sure what I should do for further verification. And of course, how the code patch can be reviewed?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to andy.zsshen from comment #5)
> Hi, I am new to the SpiderMonkey engine.
> 
> After some study for the comment #0 and the document, I already wrote the
> bailout code for StringReplace operation. Besides, for the debug build
> testing (~/mozilla-central/js/src/jit-test/jit_test.py
> ~/mozilla-central/js/src/build-debug/dist/bin/js), all the cases are passed.
> 
> But I am not sure what I should do for further verification. And of course,
> how the code patch can be reviewed?

Unfortunately, we do not have any way to enforce that a recover instruction is welll working for the moment, except verifying in IonGraph.  You can find some information about it on the Hacking tips page [1], and in Bug 1003801.

You have a short guide to SpiderMonkey contributions [2], the bottom part describes how to use Mercurial for making patches.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_%28JS_shell%29
[2] https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey (see also https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F )

Comment 7

[andy.zsshen]

Attachment: The implementation of StringReplace recover instruction

A preliminary triage to join the mentor bug.

Comment 8

[andy.zsshen]

Attachment: Hi Nicolas! It is a preliminary trial for the StringReplace recover instruction. Could you help to review the patch?

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8518153 [details] [diff] [review]
Hi Nicolas! It is a preliminary trial for the StringReplace recover instruction. Could you help to review the patch?

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

This patch looks good.
Can you add test, in a similar as done for RRegExpTest?  Check with both the global and the sticky bit of regexps.

Comment 10

[andy.zsshen]

Attachment: Hi, here are the DCE test cases for StringReplace.

The test cases for StringReplace recover instruction.

Comment 11

[andy.zsshen]

Attachment: IonGraph for rstring_replace_func206-pass16-DCE-mir.gv.png

Comment 12

[andy.zsshen]

Attachment: IonGraph for rstring_replace_y_func207-pass16-DCE-mir.gv.png

Comment 13

[andy.zsshen]

Attachment: IonGraph for rstring_replace_g_func208-pass16-DCE-mir.gv.png

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8519500 [details] [diff] [review]
Hi, here are the DCE test cases for StringReplace.

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

Merge these test cases with the previous patch, and ask for review.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +929,5 @@
>  
> +var uceFault_string_replace = eval(uneval(uceFault).replace('uceFault', 'uceFault_string_replace'))
> +function rstring_replace(i) {
> +    var re = /str\d+9/;
> +    var res = "str00123456789".replace(re, "abc");

Assert that re.lastIndex hold before and after the call to String_replace.

@@ +940,5 @@
> +
> +var uceFault_string_replace_y = eval(uneval(uceFault).replace('uceFault', 'uceFault_string_replace_y'))
> +function rstring_replace_y(i) {
> +    var re = /str\d+9/y;
> +    var res = "str00123456789".replace(re, "abc");

same here.

@@ +951,5 @@
> +
> +var uceFault_string_replace_g = eval(uneval(uceFault).replace('uceFault', 'uceFault_string_replace_g'))
> +function rstring_replace_g(i) {
> +    var re = /str\d+9/g;
> +    var res = "str00123456789str00123456789".replace(re, "abc");

and same here.

Comment 15

[andy.zsshen]

Attachment: Merge the StringReplace recover instruction and the corresponding test cases.

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8520720 [details] [diff] [review]
Merge the StringReplace recover instruction and the corresponding test cases.

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

This looks good :)
I will push this patch to our try server.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +929,5 @@
>  
> +var uceFault_string_replace = eval(uneval(uceFault).replace('uceFault', 'uceFault_string_replace'))
> +function rstring_replace(i) {
> +    var re = /str\d+9/;
> +    

nit: remove this trailing whitespace.

@@ +943,5 @@
> +
> +var uceFault_string_replace_y = eval(uneval(uceFault).replace('uceFault', 'uceFault_string_replace_y'))
> +function rstring_replace_y(i) {
> +    var re = /str\d+9/y;
> +    

nit: same here.

Comment 17

[Nicolas B. Pierron [:nbp]]

Can you make a patch, instead of only attaching a diff, such as I can use your user name as you want it to be displayed.

https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 18

[Nicolas B. Pierron [:nbp]]

Here is the link to our continuous integration infrastructure.  If everything is green, then this patch would be good for landing in Firefox.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dfc4569009eb

Comment 19

[andy.zsshen]

Attachment: Formal patch for StringReplace recover instruction and the corresponding test cases.

Comment 20

[Nicolas B. Pierron [:nbp]]

After all, I do not think this patch is capable of the Compacting GC issue seen there, and the error looks more like an old issue which got reported by jonco a few weeks ago.  It might just be possible that I pushed the previous patch on top of a non-fixed branch.

So, I pushing again on top of a more recent branch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efd9d48a012d

Comment 21

[andy.zsshen]

Hi! Unfortunately, the tests in the treeherder were all busted.

It showed the compilation error:

"src/js/src/jit/MIR.h:6953:21: error: 'pattern' was not declared in this scope."

But I think the error is not caused by the StringReplace patch.

Is there something wrong?

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to andy.zsshen from comment #21)
> But I think the error is not caused by the StringReplace patch.
> 
> Is there something wrong?

Yes, this sadly looks like a rebase issue, I wonder why.
In the mean time I made another push to try, sorry for these round-trips :/

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c29a345a3bef

Comment 23

[Nicolas B. Pierron [:nbp]]

Finally, and Congratulation \o/

Your patch is now on mozilla-inbound, which means that it would be available in Nightly version of Firefox in a few hours, and that it would be available to everybody using Firefox 36 ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/293af54d2872

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/293af54d2872