Closed Bug 1272269 Opened 8 years ago Closed 8 years ago

20% performance regression since Aurora48, decrypting with pbkdf2 algorithm.

Categories

(Core :: JavaScript Engine: JIT, defect)

48 Branch
x86
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed

People

(Reporter: alice0775, Assigned: h4writer)

References

Details

(Keywords: perf, regression)

Attachments

(2 files)

[Tracking Requested - why for this release]: 20% performance regression

+++ This bug was initially created as a clone of Bug #1271947 +++

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/674a552743785c28c75866969aad513bd8eaf6ae
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160511030221

When I test Bug 1271947, I found a performance regression.


Steps to reproduce:
1. Open attachment 8751602 [details]
2. Click [Test] several times


Actual results:
20% performance regression.

approx. 1000 : Beta47.0b4
approx. 1200 : Nightly49.0a1, Aurora48.0a2



Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=beca0a8904718c01dfe57a758ce2ceccc028dc2b&tochange=7c1f8d3d4f69add3995d27d4c70c92d286aa54b9
Taking
Flags: needinfo?(hv1989)
Assignee: nobody → hv1989
Blocks: 1241088
Flags: needinfo?(hv1989)
Attached patch PatchSplinter Review
During the refactor I forgot to notice that the NewArray/NewObject vm variants also use the template object to improve performance. I did remove that. This wasn't an issue, since in all those cases we should now be using the shared stubs variant. Except for the "new Array()" call, which cannot use a shared stub. Adding the template object to the all vm calls again (even if this should only be visible on newArrayTryVM called from MCallOptimize.cpp)
Attachment #8751747 - Flags: review?(efaustbmo)
Comment on attachment 8751747 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +7278,5 @@
>  
>      // Emit a VM call.
>  
>      gc::InitialHeap heap = gc::DefaultHeap;
> +    MConstant* templateConst = constant(NullValue());

make this again:
MConstant* templateConst = MConstant::New(alloc(), NullValue());

constant already does the "current->add"

@@ +7417,3 @@
>  
>      gc::InitialHeap heap = gc::DefaultHeap;
> +    MConstant* templateConst = constant(NullValue());

make this again:
MConstant* templateConst = MConstant::New(alloc(), NullValue());

constant already does the "current->add"
Alice: thanks for taking the time to look into this!
Tracking for 48/49 based on the performance regression.
Thank you, Alice!
Comment on attachment 8751747 [details] [diff] [review]
Patch

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

r=me with the MConstant stuff you mention taken care of.
Attachment #8751747 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/ac69d4a75d6c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Hi Eric, do you want to uplift this for 48 if these patches are not too risky? If these patches are risky, we might need QE's help to verify.
Flags: needinfo?(efaustbmo)
Attached patch Aurora patchSplinter Review
It was my intention to request approval for aurora. Thanks for the reminder!

Approval Request Comment
[Feature/regressing bug #]:
bug 1241088

[User impact if declined]:
Performance could be lower when using new Array() in FF48 only

[Describe test coverage new/current, TreeHerder]:
Mozilla-inbound for already 3-4 days

[Risks and why]: 
Quite low. It reintroduces an optimization path that was removed by bug 1241088. Made the patch as small as possible.

[String/UUID change made/needed]:
/
Flags: needinfo?(efaustbmo)
Attachment #8753831 - Flags: review+
Attachment #8753831 - Flags: approval-mozilla-aurora?
Comment on attachment 8753831 [details] [diff] [review]
Aurora patch

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

The patch fixes the performance issue. Take it in aurora.
Attachment #8753831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.