Closed Bug 1102240 Opened 10 years ago Closed 6 years ago

Split inspector.js into smaller files.

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Version:
33 Branch
Platform:
x86_64
Linux
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: tomasz, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1102240 - Move inspector actor to dedicated folder;
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1102240 - split inspector actor in smaller files;
59 bytes, text/x-review-board-request
pbro
: review+
ochameau
: review+
Details
Bug 1102240 - rename actors/inspector/inspector.js to inspector-actor.js;
59 bytes, text/x-review-board-request
pbro
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141115160330

Steps to reproduce:

This is split out of bug 1094622 comment 17.

Inspector.js is over 3000 lines long as is becoming the new browser.js. Refactor it before it's too late. Bug 1094622 comment 17 has some suggestions as what logical pieces may have sense.
Depends on: 1094622
Component: Untriaged → Developer Tools: Inspector
Filter on CLIMBING SHOES
Severity: normal → enhancement
Priority: -- → P3
This fits perfectly Bug 1432780 and I already have the refactor ready. Taking this one.
Assignee: nobody → jdescottes
Blocks: split-devtools-actors
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8945773 [details]
Bug 1102240 - Move inspector actor to dedicated folder;

https://reviewboard.mozilla.org/r/215884/#review221670

Simple move. Looks good. Thanks.
Attachment #8945773 - Flags: review?(pbrosset) → review+
Comment on attachment 8945775 [details]
Bug 1102240 - rename actors/inspector/inspector.js to inspector-actor.js;

https://reviewboard.mozilla.org/r/215888/#review221672
Attachment #8945775 - Flags: review?(pbrosset) → review+
Some context: I think the shadow root work ongoing in Bug 1053898 is a good opportunity to cleanup a bit out walkers & actors. This is why I want to push for this refactor right now. Also we are at the beginning of the release, so it's a good time to land this kind of changes.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=db2b30bd4c65dc4e542a502478c8d7fcffeba6b1 (had a green try already, but with many leftover commits)
Comment on attachment 8945774 [details]
Bug 1102240 - split inspector actor in smaller files;

https://reviewboard.mozilla.org/r/215886/#review221678

Thanks Julian. This will make working on this code so much easier.
I was very confused at first with this diff, because the original code appears in multiple places. But copying the file and removing the unnecessary parts seems like the right way to do this while preserving history.
So, all good for me.
Attachment #8945774 - Flags: review?(pbrosset) → review+
Thanks for the reviews!

I haven't seem a performance impact when running DAMP locally before/after this patch.
Talos push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3423e3910d23208ef588501ba3013d50aac4a27b
Here's a talos compare between my patches and the revision on top of which I was working:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=4dde19dc8f57772e7bd745db5adb1ea5911b7f6e&newProject=try&newRevision=3423e3910d23208ef588501ba3013d50aac4a27b&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

Notable:
- inspector.mutations get 10% faster with high confidence (?)
- custom.inspector.reload.settle.DAMP is slower (20%) but the variance is huge, so the confidence is low. I will retrigger the jobs a few times.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #11)
> Here's a talos compare between my patches and the revision on top of which I
> was working:
> 
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=try&originalRevision=4dde19dc8f57772e7bd745db5
> adb1ea5911b7f6e&newProject=try&newRevision=3423e3910d23208ef588501ba3013d50aa
> c4a27b&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignatur
> e=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=1728
> 00
> 
> Notable:
> - inspector.mutations get 10% faster with high confidence (?)

Any random guess explaining why it is significantly faster?

> - custom.inspector.reload.settle.DAMP is slower (20%) but the variance is
> huge, so the confidence is low. I will retrigger the jobs a few times.

All results with low confidence should be ignored. So you can safely ignore that.

I'll review the patch on monday.
Thanks for the feedback!

> Any random guess explaining why it is significantly faster?

I really don't know. One of the things that is stressed by the test is the instantiation of DocumentWalker. The test will create thousands of them. Now that DocumentWalker is isolated in a dedicated file, maybe calling new DocumentWalker() is not as heavy as it used to be. The created closure should be smaller, now how does this impact performance, I am not sure. 

I will get the profiles to understand this a bit better. I can also try to push a change that only moves DocumentWalker.
Actually no mystery here, I made a mistake. I got rid of the default params of the DocumentWalker because I thought they were always provided. But it's not the case. So of course the test was faster because no filter was running (and filters are a slow part of using the walkers).

I reverted this error, and locally the mutations test no longer show any improvement.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0548ee03f27b2d835ad9fe749790681c4267ab
talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d2cb64100daaf5efb057de488762716a85db4f

Interesting to note, server tests caught the issue but my first try run was only running devtools mochitests. The server mochitests are running in other chunks, (here c1) so I didn't notice the failures. Glad it's covered though!
Comment on attachment 8945775 [details]
Bug 1102240 - rename actors/inspector/inspector.js to inspector-actor.js;

https://reviewboard.mozilla.org/r/215888/#review221784


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/inspector/document-walker.js:182
(Diff revision 3)
>    },
>  };
>  
>  exports.DocumentWalker = DocumentWalker;
> +exports.SKIP_TO_PARENT = SKIP_TO_PARENT;
> +exports.SKIP_TO_SIBLING = SKIP_TO_SIBLING;

Error: Newline required at end of file but not found. [eslint: eol-last]
Comment on attachment 8945774 [details]
Bug 1102240 - split inspector actor in smaller files;

https://reviewboard.mozilla.org/r/215886/#review222320

Thanks for this clean!

Sorry for the delay, you would have to rebase against bug 1432851.

::: devtools/server/actors/inspector/utils.js:261
(Diff revision 4)
>      }
>    };
>  });
> +
> +module.exports = {
> +  allAnonymousContentTreeWalkerFilter,

nit: This is only used by WalterActor, you may inline it into walkor-actor.js.

::: devtools/server/actors/inspector/utils.js:262
(Diff revision 4)
>    };
>  });
> +
> +module.exports = {
> +  allAnonymousContentTreeWalkerFilter,
> +  ensureImageLoaded,

There is no need to export 'ensureImageLoaded', this isn't used outside of utils.js.

::: devtools/server/actors/inspector/utils.js:266
(Diff revision 4)
> +  allAnonymousContentTreeWalkerFilter,
> +  ensureImageLoaded,
> +  getNodeDisplayName,
> +  imageToImageData,
> +  isNodeDead,
> +  isWhitespaceTextNode,

Like ensureImageLoaded, "isWhitespaceTextNode" is only used by utils.js and doesn't need to be exported.

::: devtools/server/actors/inspector/utils.js:268
(Diff revision 4)
> +  getNodeDisplayName,
> +  imageToImageData,
> +  isNodeDead,
> +  isWhitespaceTextNode,
> +  nodeDocument,
> +  nodeDocshell,

nit: 'nodeDocshell' is only used by walker-actor and can be inline in it.

::: devtools/server/actors/inspector/utils.js:269
(Diff revision 4)
> +  imageToImageData,
> +  isNodeDead,
> +  isWhitespaceTextNode,
> +  nodeDocument,
> +  nodeDocshell,
> +  nodeHasSize,

'nodeHasSize' is only used by utils.js and doesn't need to be exported.

::: devtools/server/tests/mochitest/test_inspector-mutations-value.html:24
(Diff revision 4)
>    SimpleTest.waitForExplicitFinish();
>    runNextTest();
>  };
>  
>  const testSummaryLength = 10;
> -inspector.setValueSummaryLength(testSummaryLength);
> +WalkerActor.setValueSummaryLength(testSummaryLength);

Hum. This test doesn't involve e10s as doing this would fail. I imagine it runs with non-e10s codepath because the document we load in a tab:
https://searchfox.org/mozilla-central/source/devtools/server/tests/mochitest/test_inspector-mutations-value.html#158
is most likely a chrome:// URL and they are loaded in parent process.

This has nothing to do with your patch, but may be we should do something about that?
Attachment #8945774 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Thanks for this clean!
                       ^+up ;)
Thanks for the review! 

Followed all your suggestions except for allAnonymousContentTreeWalkerFilter which I kept in utils, to keep it next to the other filter. 

Updated try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c02a6c155d2fb85875877e40731dd7e2e5d6bec

(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Comment on attachment 8945774 [details]
> Bug 1102240 - split inspector actor in smaller files;
>
> ::: devtools/server/tests/mochitest/test_inspector-mutations-value.html:24
> (Diff revision 4)
> >    SimpleTest.waitForExplicitFinish();
> >    runNextTest();
> >  };
> >  
> >  const testSummaryLength = 10;
> > -inspector.setValueSummaryLength(testSummaryLength);
> > +WalkerActor.setValueSummaryLength(testSummaryLength);
> 
> Hum. This test doesn't involve e10s as doing this would fail. I imagine it
> runs with non-e10s codepath because the document we load in a tab:
> https://searchfox.org/mozilla-central/source/devtools/server/tests/mochitest/
> test_inspector-mutations-value.html#158
> is most likely a chrome:// URL and they are loaded in parent process.
> 
> This has nothing to do with your patch, but may be we should do something
> about that?

Those are chrome mochitests. So as you said, everything runs in the the parent process, even if e10s is on. Knowing that I don't find the test particularly bad, is there something in particular that feels fragile here?

Maybe a related question is should we migrate those tests to be browser mochitests? Not sure, this allows us to test the server without involving DevTools UI code. At least for mutations we have integration tests in devtools/client/inspector/markup/test/ to check that we handle mutations properly from a UI standpoint.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #30)
> Those are chrome mochitests. So as you said, everything runs in the the
> parent process, even if e10s is on.

This isn't related to them being chrome mochitests.
It is mainly because they are using a test document served from chrome:// URI.
I think we will use the e10s codepath if make them use the http server instanciated by mochitest.
(if the http server is also started for chrome mochitests)

> Knowing that I don't find the test particularly bad, is there something in particular that feels fragile here?

The fact that we are using a codepath that is no longer used.
You can no longer disable e10s in Firefox. So we are testing something that isn't used in production
and we are not testing the important codepath actually used in production.

> Maybe a related question is should we migrate those tests to be browser
> mochitests? Not sure, this allows us to test the server without involving
> DevTools UI code. At least for mutations we have integration tests in
> devtools/client/inspector/markup/test/ to check that we handle mutations
> properly from a UI standpoint.

Yes, in an ideal world, I think we should convert them all to browser mochitests.
But the main reason to do that would be consistency/simplicity.
chrome mochitests don't have much value compared to browser mochitests.
It would be easier to maintain and document only one main test suite.
The same argument would apply to xpshell, but this is slightly different as xpshell has some value:
much faster to run, can run in parallel and have less noise (nothing runs but your test, no firefox, no addons, ...).
(In reply to Alexandre Poirot [:ochameau] from comment #31)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #30)
> > Those are chrome mochitests. So as you said, everything runs in the the
> > parent process, even if e10s is on.
> 
> This isn't related to them being chrome mochitests.
> It is mainly because they are using a test document served from chrome://
> URI.
> I think we will use the e10s codepath if make them use the http server
> instanciated by mochitest.
> (if the http server is also started for chrome mochitests)

Right. Interesting to note that e10s is fully disabled for all chrome mochitests:
https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/testing/mochitest/runtests.py#2538

I assumed this was running in e10s but with both client and server in the parent process. Which was fine IMO, because this is how most of our about:pages are running.

> 
> > Knowing that I don't find the test particularly bad, is there something in particular that feels fragile here?
> 
> The fact that we are using a codepath that is no longer used.
> You can no longer disable e10s in Firefox. So we are testing something that
> isn't used in production
> and we are not testing the important codepath actually used in production.
> 

This is devtools server code running against content. Are we using different codepaths for devtools server code in e10s/non-e10s?

> > Maybe a related question is should we migrate those tests to be browser
> > mochitests? Not sure, this allows us to test the server without involving
> > DevTools UI code. At least for mutations we have integration tests in
> > devtools/client/inspector/markup/test/ to check that we handle mutations
> > properly from a UI standpoint.
> 
> Yes, in an ideal world, I think we should convert them all to browser
> mochitests.
> But the main reason to do that would be consistency/simplicity.
> chrome mochitests don't have much value compared to browser mochitests.

Their value is that they don't involve the UI, so if we break something on the server, server-only tests should do a better job to highlight what regressed. That is, if we actually write them when needed.

> It would be easier to maintain and document only one main test suite.

I think there is value in having different layers of testing. But in practice we mostly write browser mochitests + occasional xpcshell tests. Having an additional test suite to maintain might not be worth it. 

Are you feeling the same about all chrome mochitest, or is this one a special case because it opens an additional tab to inspector-traversal-data.html (I don't know why this would change the cons you listed).

> The same argument would apply to xpshell, but this is slightly different as
> xpshell has some value:
> much faster to run, can run in parallel and have less noise (nothing runs
> but your test, no firefox, no addons, ...).

As you said, the discussion should happen outside of this bug. I don't feel like there is a strong urge to migrate out of chrome mochitests.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c42b9b8bd98
Move inspector actor to dedicated folder;r=pbro
https://hg.mozilla.org/integration/autoland/rev/8e3c63c83301
split inspector actor in smaller files;r=ochameau,pbro
https://hg.mozilla.org/integration/autoland/rev/17a0e7ddfb48
rename actors/inspector/inspector.js to inspector-actor.js;r=pbro
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: