1420130 - Remove React Proxy Monkeypatch and see how it affects DAMP

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We have serious doubts whether our React monkeypatch is still needed and think it may be negatively impacting performance.

We need to remove the monkeypatch and see how this affects DAMP.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1420130 - Remove React Proxy Monkeypatch and see how it affects DAMP

Review commit: https://reviewboard.mozilla.org/r/202498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202498/

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Looking at the try run we are missing something.

Comment 3

[Alexandre Poirot [:ochameau]]

At first sight, DAMP reports no difference with this removal.
It is surprising, I would have expected something to happen in the inspector...

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> At first sight, DAMP reports no difference with this removal.
> It is surprising, I would have expected something to happen in the
> inspector...

That is because most tests failed... I am trying to work out why at the moment.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

In order for events to work properly inside XUL documents we still need the proxy.

We should check what happens to performance when we use the correct URLS. At the moment we use:

chrome://devtools/content/inspector/inspector.xhtml
chrome://devtools/content/netmonitor/netmonitor.xhtml
chrome://devtools/content/webconsole/webconsole.xhtml

Here are some corrected URLS:

chrome://devtools/content/inspector/inspector.xhtml
chrome://devtools/content/memory/memory.xhtml
chrome://devtools/content/netmonitor/index.html
chrome://devtools/content/dom/dom.html

Let's fix the URLs and see how this affects DAMP.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8931352 [details]
Bug 1420130 - Remove React Proxy Monkeypatch and see how it affects DAMP

Please r? again once you get some meaningful results.

Comment 7

[Alexandre Poirot [:ochameau]]

FYI, I tried fixing these URLs (in order to really use this monkey patch) and got very bad performance regressions:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=a333d65451d01212490ae3d4354a3782253de45d&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

Also, see the two additional patch included in this try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a333d65451d01212490ae3d4354a3782253de45d
We are missing `commonLibRequire` argument in BrowserLoader to really reuse react between panels and toolbox.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Removing the monkeypatch breaks things like sidebar section header clicking (accordian style) because events that fire in a XUL iframe propagate up to the parent frame but this does not happen when HTML is in the mix.

I guess the solution would be to switch over to fully using HTML but some of the performance drops in your tests are 20 - 30%... if converting fully to HTML would give us similar performance increases we should probably consider it.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #8)
> Removing the monkeypatch breaks things like sidebar section header clicking
> (accordian style) because events that fire in a XUL iframe propagate up to
> the parent frame but this does not happen when HTML is in the mix.

What is the XUL iframe in this case - the iframe inside of toolbox.xul pointing to inspector.xhtml?

> I guess the solution would be to switch over to fully using HTML but some of
> the performance drops in your tests are 20 - 30%... if converting fully to
> HTML would give us similar performance increases we should probably consider
> it.

FWIW I think the main blockers from switching the toolbox from a xul doc to an html doc are all either now fixed or fixable (tracked in Bug 1178254). Things that would still need addressing, I think, are:
* The <splitter#toolbox-console-splitter> would have to be converted to a SplitBox component if we wanted to switch to HTML (although we could maybe keep using it as a xul:splitter if we switched first to xhtml).
* Figure out if we still need editMenuOverlay.xul

After that I think we'd want to create a toolbox-wrapper.xul that has a full width/height iframe pointing to toolbox.(x)html, and is used for creating menus or other things that require a XUL doc (like https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/menu.js#65).

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have been looking into this and the only issue when disabling the monkeypatch is that the accordian headers in the inspector side panel (e.g. grid an box model) become unclickable... interestingly though, the rule view works just fine.

In the box model view the Box Model header ignores clicks but the box model itself doesn't.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1420130 - Remove React Proxy Monkeypatch

Review commit: https://reviewboard.mozilla.org/r/219880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219880/

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Doing a try run to see if my new patch breaks anything... in real life everything seems fine but when running tests you never truly know.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8950605 [details]
Bug 1420130 - Remove React Proxy Monkeypatch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219880/diff/1-2/

Comment 14

[Greg Tatum [:gregtatum] (REO Fx132)]

I'll do this review first thing tomorrow morning. I didn't get to it today.

Comment 15

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: monkeypatch.png

It looks like they are no longer doing their synthetic event dispatch system. The events are bound to the actual elements, not to the window elements.

Comment 16

[Greg Tatum [:gregtatum] (REO Fx132)]

Comment on attachment 8950605 [details]
Bug 1420130 - Remove React Proxy Monkeypatch

https://reviewboard.mozilla.org/r/219880/#review226566

The patch itself looks reasonable, but I'd prefer to see all of the mochitests run before giving an r+. I'm only seeing the talos test on the patch. Could you provide a link to a full test run? All of my manual testing shows that they've changed their approach for event listeners, so it does indeed look like we can remove the monkeypatch!

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #16)
> Comment on attachment 8950605 [details]
> Bug 1420130 - Remove React Proxy Monkeypatch and see how it affects DAMP
> 
> https://reviewboard.mozilla.org/r/219880/#review226566
> 
> The patch itself looks reasonable, but I'd prefer to see all of the
> mochitests run before giving an r+. I'm only seeing the talos test on the
> patch. Could you provide a link to a full test run? All of my manual testing
> shows that they've changed their approach for event listeners, so it does
> indeed look like we can remove the monkeypatch!

That was this try run... all green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20620b2fe584fffeeed1a40327be69e921f3b66f

Comment 18

[Greg Tatum [:gregtatum] (REO Fx132)]

Comment on attachment 8950605 [details]
Bug 1420130 - Remove React Proxy Monkeypatch

https://reviewboard.mozilla.org/r/219880/#review227642

Thanks for sharing the try run, everything looks good to me!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84ab509808e7
Remove React Proxy Monkeypatch and see how it affects DAMP r=gregtatum

Comment 20

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failures on /browser_fontinspector_expand-css-code.js

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=163303830&repo=autoland&lineNumber=2273
Push: https://hg.mozilla.org/integration/autoland/rev/84ab509808e785f6408b97d594aba6c1f2fa7ac7
Backout: https://hg.mozilla.org/integration/autoland/rev/aee6592ece6b79436913d2a50d72131d3d80fce3

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8950605 [details]
Bug 1420130 - Remove React Proxy Monkeypatch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219880/diff/2-3/

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8950605 [details]
Bug 1420130 - Remove React Proxy Monkeypatch

https://reviewboard.mozilla.org/r/219880/#review227956

Font Inspector changes between try and landing caused failing tests. Just a couple more `event.stopPropagation()` calls were needed. I will push to try again and land if it is green.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8950605 [details]
Bug 1420130 - Remove React Proxy Monkeypatch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219880/diff/3-4/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d17b9126dca6
Remove React Proxy Monkeypatch r=gregtatum

Comment 25

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d17b9126dca6