Closed Bug 1454580 Opened 2 years ago Closed 2 years ago

Add DAMP test to track RDP/protocol.js performance

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

(Whiteboard: dt-fission)

Attachments

(1 file)

Bug 1449162 proved that protocol.js was slower than old fashion actors.
I started working on patches to optimize protocol.js, but they don't always translate into improvements in DAMP.
A benchmark test, involving a lot of heavy loaded messages could help fixing smaller performance improvements.
Summary: Add DAMP test to track RDP/protocol.js performancec → Add DAMP test to track RDP/protocol.js performance
Assignee: nobody → poirot.alex
The test in the current state isn't perfect as it doesn't react to recent improvements I've been working on.

The two patches from bug 1454373 don't triger any improvement in the new test:
* Switch protocol.js to native promises:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b390f76668c8&newProject=try&newRevision=220b12cdc56bf779d82d8d6619cf6e063ddc9301&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=server&framework=1
2,471.27 ± 0.74% 	> 	2,457.50 ± 0.65% 	-0.56% (low)

* Make defer module use DOM Promises:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=220b12cdc56bf779d82d8d6619cf6e063ddc9301&newProject=try&newRevision=21e874398cd7c5ed0188a93af56f22d437326ed7&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=server&framework=1
2,457.50 ± 0.65% 	> 	2,427.43 ± 0.96% 	-1.22% (low)

But a couple of small patches I have against protocol.js (bug 1449162 comment 47), that don't trigger any improvement in current DAMP tests, now trigger a very significant win on this new test:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=21e874398cd7&newProject=try&newRevision=a1afa2b040a46c306b846e3b22ec9d019b3ebbe9&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=server&showOnlyConfident=1&framework=1
2,427.43 ± 0.96% 	> 	1,110.36 ± 0.78% 	-54.26% (high)
Ah, but if I revert bug 1454373, DAMP fails with "allocation size overflow":
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b303f2b2655bebaa148ce468f2630d420e7b93ee&selectedJob=174045730
So at least it would have identified such regression...
It also highlights how bad Promise.jsm was as the DAMP test isn't so intense!
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Ah, but if I revert bug 1454373, DAMP fails with "allocation size overflow":

Sorry I mixed bugs here. I reverted bug 1453385 here.

> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b303f2b2655bebaa148ce468f2630d420e7b93ee&selectedJob=1
> 74045730
> So at least it would have identified such regression...
> It also highlights how bad Promise.jsm was as the DAMP test isn't so intense!

So it isn't highlighting how bad Promise.jsm, but how bad Response.write was!
(In reply to Alexandre Poirot [:ochameau] from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > Ah, but if I revert bug 1454373, DAMP fails with "allocation size overflow":
> 
> Sorry I mixed bugs here. I reverted bug 1453385 here.

Even if I reduce the number of method calls from 1000 to 10, it still allocate too much memory:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=450f165594199200955f50c76716078f56545a7c
So this patch definitely has a very significant impact on memory usage of protocol.js!
Comment on attachment 8968428 [details]
Bug 1454580 - Add a DAMP test to watch RDP/protocol.js performance.

https://reviewboard.mozilla.org/r/237124/#review245936

Test itself looks resonable; thanks for adding it! Not sure what to say about the effects on other tests... Subtle effects of things happening in a different order... :S
Attachment #8968428 - Flags: review?(jryans) → review+
So here is some data to see how each test parameter change the overall test runtime:

* 10-1000-50-1000 => 6,086.55ms [increase repeat]
* 10-1000-100-500 => 5,399.73ms [increase array size]
* 20-1000-50-500 => 5,301ms  [increase attributes number]
* 10-1000-50-500 => 2,790.28ms [reference]
* 10-500-50-500 => 2,527.26ms [reduce string size]

These dashed numbers represent the test parameters like his:
  Attributes# - String size - Array size - Repeat

The conclusion of this is that:
 * Attribute number, array size and repeat have a very significant impact
 * String size doesn't have a big impact
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f88c817b334
Add a DAMP test to watch RDP/protocol.js performance. r=jryans
https://hg.mozilla.org/mozilla-central/rev/8f88c817b334
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: dt-pjs
Product: Firefox → DevTools
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.