Closed Bug 1066040 Opened 5 years ago Closed 5 years ago

IonMonkey: Implement StringReplace recover instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: nbp, Assigned: andy.zsshen, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(4 files, 4 obsolete files)

Implement StringReplace in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
See Bug 1050649 for a similar instruction.
Whiteboard: [good first bug][lang=c++]
Assignee: nobody → anujagarwal464
Assignee: anujagarwal464 → nobody
@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.
Attachment #8517546 - Flags: review?(andy.zsshen)
Assignee: nobody → andy.zsshen
Status: NEW → ASSIGNED
Attachment #8517546 - Attachment is obsolete: true
Attachment #8517546 - Flags: review?(andy.zsshen)
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.
Attachment #8518153 - Flags: review?(nicolas.b.pierron) → feedback+
The test cases for StringReplace recover instruction.
Attachment #8519500 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8519500 - Flags: review?(nicolas.b.pierron) → feedback+
Attachment #8520720 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8520720 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8518153 - Attachment is obsolete: true
Attachment #8519500 - Attachment is obsolete: true
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
Attachment #8522211 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8520720 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/293af54d2872
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.