1352168 - 5.11% dromaeo_dom (linux64) regression on push a58cea09759988011ac5ca328992a272c06fbcf4 (Wed Mar 29 2017)

Status: RESOLVED DUPLICATE of bug 1353629

(Core :: DOM: Core & HTML, defect, P2)

Description

[Robert Wood [:rwood]]

Talos has detected a Firefox performance regression from push a58cea09759988011ac5ca328992a272c06fbcf4. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  5%  dromaeo_dom summary linux64 opt      1594.79 -> 1513.25


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=5747

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

Comment 1

[Robert Wood [:rwood]]

Hi Andrea, would you please be able to confirm that this performance regression is a result of the patch in Bug 1340921? Thanks!

Comment 2

[Andrea Marchesini [:baku]]

Yes, definitely this patch causes a memory regression. This patch is a temporary fix for enabling multi-e10s in aurora.
A bit of context: when a memory blob is sent from content to parent process, if the memory size is < 1mb, we send the full content as 1 single IPC message. Otherwise, we used to send a PChildToParentStream actor, and, only when needed, the content of that blob was sent in chunks.

The result was that, on the parent side, the nsIInputStream was a nsIPipeStream, This kind of stream is not serializable, and not seekable and, because of that, it is incompatible with multi-e10s.

My approach was to create a new kind of actor: PMemoryStream, where the content is sent in chunks via IPC immediately. On the parent side, those chunks are merged in 1 single stream, seekable and serializable. This is possible because the content is immediately available before any additional operation. Of course, this requires more data to be transfered, more memory allocation, and so on.

In the meantime, I'm working on a refactoring of this. It will take a while, but it is my top priority. I hope to finish it in 1 or 2 weeks time. So, the current setup is a 'temporary' fix for multi-e10s in aurora. A better approach will be available asap.

Comment 3

[Robert Wood [:rwood]]

> In the meantime, I'm working on a refactoring of this. It will take a while,
> but it is my top priority. I hope to finish it in 1 or 2 weeks time. So, the
> current setup is a 'temporary' fix for multi-e10s in aurora. A better
> approach will be available asap.

Ok great, thank you Andrea for the detailed reply, appreciated!

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Robert Wood [:rwood] from comment #3)
> > In the meantime, I'm working on a refactoring of this. It will take a while,
> > but it is my top priority. I hope to finish it in 1 or 2 weeks time. So, the
> > current setup is a 'temporary' fix for multi-e10s in aurora. A better
> > approach will be available asap.
> 
> Ok great, thank you Andrea for the detailed reply, appreciated!

Have we got any chance to have the better approach landed yet?

Comment 5

[Andrea Marchesini [:baku]]

I think this is already fix by IPCBlob refactoring. I get rid of PMemoryStream and so on.