Nicolas B. Pierron [:nbp] Reporter
	Description • 10 years ago

Implement RStringLength in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

	Sebastian Holzapfel [:schnommus] Assignee
	Comment 1 • 10 years ago

I've been attempting to implement this; first time I've ever worked on any Mozilla code so flailing in the dark a fair bit.

The code passes the test case I've written, but there are a few things I'm not sure about:

- Whether my testcase is any good; the other test cases in the file seem to have assertions that pass ONLY when i is 99; mine would technically pass for some other values of i (even though the test is performed on 99). Mostly did this to avoid having to use other rops that haven't been implemented yet.
- Whether the code is even executing: I had a look with iongraph at the JIT DCE dump of functions with rops that had been implemented vs. those that hadn't, and I couldn't notice any difference (I'm probably looking in the wrong spot)
- Whether there's anything horrific I've done in the code

	Nicolas B. Pierron [:nbp] Reporter
	Comment 2 • 10 years ago

(In reply to Sebastian Holzapfel [:schnommus] from comment #1)
> Created attachment 8440361 [details] [diff] [review]
> bug1020638_stringlength_recover.diff
> 
> I've been attempting to implement this; first time I've ever worked on any
> Mozilla code so flailing in the dark a fair bit.
> 
> The code passes the test case I've written, but there are a few things I'm
> not sure about:
> 
> - Whether my testcase is any good; the other test cases in the file seem to
> have assertions that pass ONLY when i is 99; mine would technically pass for
> some other values of i (even though the test is performed on 99). Mostly did
> this to avoid having to use other rops that haven't been implemented yet.

This sounds good.

> - Whether the code is even executing: I had a look with iongraph at the JIT
> DCE dump of functions with rops that had been implemented vs. those that
> hadn't, and I couldn't notice any difference (I'm probably looking in the
> wrong spot)

One way to test is to run the test case under gdb, and check if the code of the recover function is used.  In iongraph, the StringLength instruction should appear in gray, does it?

Also, this test case is made to exercise the optimized path, and check that we do not damage JS semantic if we do optimize it. It does not test if we are indeed optimizing it.

	Nicolas B. Pierron [:nbp] Reporter
	Comment 3 • 10 years ago

Comment on attachment 8440361 [details] [diff] [review]
bug1020638_stringlength_recover.diff

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

This patch sounds good :)

I will check on my side if the test case works as expected in IonGraph.

::: js/src/jit/Recover.cpp
@@ +427,5 @@
> +    RootedValue result(cx);
> +
> +    MOZ_ASSERT(!operand.isObject());
> +    if (!js::GetLengthProperty(operand, &result ) )
> +        return false;

style-nit: avoid spaces after & before closing brackets.

@@ +429,5 @@
> +    MOZ_ASSERT(!operand.isObject());
> +    if (!js::GetLengthProperty(operand, &result ) )
> +        return false;
> +
> +    iter.storeInstructionResult( result );

style-nit: same here.

::: js/src/jit/Recover.h
@@ +35,1 @@
>      _(NewObject)                                \

style-nit: Fix indentation, do not use tabs.

	Nicolas B. Pierron [:nbp] Reporter
	Comment 4 • 10 years ago

Comment on attachment 8440361 [details] [diff] [review]
bug1020638_stringlength_recover.diff

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

::: js/src/jit/Recover.cpp
@@ +417,5 @@
>      return true;
>  }
>  
> +RStringLength::RStringLength(CompactBufferReader &reader)
> +{}

This constructor is used to decode the recover instruction, but I do not see anything for encoding it.
You will have to update MStringLength to make use of the Dead Code Elimination.

	Nicolas B. Pierron [:nbp] Reporter
	Comment 5 • 10 years ago

As you can see in this image, the StringLEngth instruction does not appear in gray, which means that even if it is listed in the resume points below, it will still have to be executed after Ion's compilation.

Also, you can notice that this instruction is Lowered correctly in the "Generate LIR" result, as it appear as:

  {[i:10]} <- stringlength (v9:r)

If the instruction is listed in gray, then this means that the instruction will not be used to generate any code during the Lowering, and that it would be added to the instruction executed when we process a resume point during a bailout.

	Sebastian Holzapfel [:schnommus] Assignee
	Comment 6 • 10 years ago

(In reply to Nicolas B. Pierron [:nbp] from comment #2-5)

Wow, thanks for all the feedback! it really does help.

I'm planning on crunching through this in the next week or so. Could I be assigned to this bug?

	Sebastian Holzapfel [:schnommus] Assignee
	Comment 7 • 10 years ago

Had another go at this, the image generated by Iongraph now seems to be correct.

	Nicolas B. Pierron [:nbp] Reporter
	Comment 8 • 10 years ago

Comment on attachment 8447564 [details] [diff] [review]
bug1020638_stringlength_recover-v2.diff

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

This sounds Good :)
I will fix the few nits which are below, and send this patch to our Try server, which test patches before we can land them to inbound.

::: js/src/jit/Recover.h
@@ +36,5 @@
>      _(CharCodeAt)                               \
>      _(FromCharCode)                             \
>      _(Pow)                                      \
>      _(PowHalf)                                  \
> +    _(StringLength)                             \

nit: In order to follow the same order of Bug 1003801, I will move it under Concat (in all files).

@@ +349,5 @@
>  
> +class RStringLength MOZ_FINAL : public RInstruction
> +{
> +public:
> +    RINSTRUCTION_HEADER_(StringLength)

style-nit: "public" keyword should be indented by 2.

	Nicolas B. Pierron [:nbp] Reporter
	Comment 9 • 10 years ago

This link [1] is the link to our test infrastructure.  I have selected a set of computers which are potentially affected by this change, and which are likely enough to catch issues if there is any.

As soon as all is green, I will land your patch on mozilla-inbound.

[1] https://tbpl.mozilla.org/?tree=Try&rev=18829c7d711f

	Nicolas B. Pierron [:nbp] Reporter
	Comment 10 • 10 years ago

Ok, the patch is now landed on inbound [1], in a few hours/days a shriff will merge these changes into mozilla-central.  When this would be the case your changes would be available in firefox Nightlies, and later in Aurora, Beta and then Release.  Congratulation :)

You can also follow the patch on mozilla-inbound and see how it is checked by our continuous integration [2].

What do you want to work on after this bug? [3]

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/933a9116ef3d
[2] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1&rev=933a9116ef3d
[3] https://bugzilla.mozilla.org/buglist.cgi?list_id=10609609&resolution=---&emailtype1=regexp&query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&email1=.*%40.*&emailbug_mentor1=1&component=JavaScript%20Engine&component=JavaScript%20Engine%3A%20JIT&component=JavaScript%3A%20GC&component=JavaScript%3A%20Internationalization%20API&component=JavaScript%3A%20Standard%20Library&product=Core

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/933a9116ef3d

	Sebastian Holzapfel [:schnommus] Assignee
	Comment 12 • 10 years ago

Thanks for fixing those nits up; I wasn't sure about positioning (should have looked at the similar bugs, and must have overlooked that).

Awesome! Happiness over here.

I do like poking around the JavaScript Engine code, so if I can find another achievable bug involved with that I'll have a go definitely. In the meantime I'll try and familiarize myself with the code a little more :)