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)
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)
13.30 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
13.37 KB,
patch
|
h4writer
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
[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
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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"
Assignee | ||
Comment 4•8 years ago
|
||
Alice: thanks for taking the time to look into this!
Comment 6•8 years ago
|
||
Thank you, Alice!
Comment 7•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac69d4a75d6c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/dfe3d2e9bdbe
You need to log in
before you can comment on or make changes to this bug.
Description
•