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

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

(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?

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?

(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 )

A preliminary triage to join the mentor bug.

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.

The test cases for StringReplace recover instruction.

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 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.

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

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

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

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?

(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

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