Move towards one actor per file for devtools/server/actors

[juliandescottes]

After working in some of our actors, I saw that a fair amount of actor files define several actors in the same file.

By looking at devtools/shared/specs/index.js, we have 42 specs files and 16 of them define more than one spec. That does not account for all actors, but confirms that this is a common pattern in the actor world.

If I take the example of the new accessibility actor, it contains the following actors: Accessible, AccessibleWalker and Accessibility. Another one, webbrowser.js, contains BrowserTabList, BrowserTabActor and BrowserAddonList. All of this feels like interesting information, but to get it you need to open and visually scan the (long) files.

I feel like having several actors (or classes) in a single file makes the code harder to navigate and understand, and would like to move each class to its own file.

I would like to hear what others think about this before filing refactor bugs. Some of us might prefer having the ThingList in the same file as the ThingActor for instance. Also there might be performance concerns to consider?

[MikeRatcliffe]

I agree that we should only have one actor per file and the file should have the same name as the actor... it makes the codebase much more maintainable and that is key, especially for new contributors.

I think the reason we often have multiple actors in a file is because if one of those actors is required all of them will be and require itself has a small cost.

[jasonLaster]

Yep, I believe Eddy was working on this refactoring slowly. It'd be helpful.

[gregtatum]

Do we know the performance cost of our loader handling lots of small files? /cc @ochameau

[jryans]

I agree breaking them up is nice for navigating the code. So like others, performance would be my main worry. Let's see what @ochameau thinks. 😁

[ochameau]

If I take the example of the new accessibility actor, it contains the following actors: Accessible, AccessibleWalker and Accessibility. Another one, webbrowser.js, contains BrowserTabList, BrowserTabActor and BrowserAddonList. All of this feels like interesting information, but to get it you need to open and visually scan the (long) files.

If you were inspired about this RFC after discovering the complexity of BrowserTabList/TabActor/AddonList. I feel that it isn't really a matter of big files, but rather that this is a particular code, with particularly abstracted patterns, where Lists classes aren't instanciated directly from RootActor and instead handed over from functions to functions. And TabActor is also a significantly complex topic because of e10s. Also various classes from old actors modules aren't actors, so it may introduce confusions if you expect all modules from actors/ directory to be actors.

Otherwise, there is an additional cost to split in many modules, for sure. The module loader has been optimized to share only one global across all DevTools modules and so only use one compartment. But it still spawn a new Sandbox, setup a new custom global object and eventually do a Filesystem request.
At the same time, splitting up files can also be a key to improve perf:

• lazy loading. In some cases, we evaluate a big files but only use a small percentage of it. Here it becomes very useful to split a module.
• loading in chunks. If you happen to mix lazy loading and user events, it allows to load less Javascript in a row. You may load 50kb at startup and 50kb on a click event, instead of 100kb on startup. It is harder to optimize like this, but this is a known issue. We often load tons of bytes of Javascript synchronously and it freeze the event loop. This is a refinement of naive lazy loading. I experimented this for the first time here: https://bugzilla.mozilla.org/show_bug.cgi?id=1365574

I would suggest looking first at lazy loading opportunities. i.e. the cases where it would really prevent loading code.
Otherwise I wouldn't make a rule of 1file=1class, but rather only split the big modules like webbrowser and inspector.

[juliandescottes]

Thanks for the feedback!

If you were inspired about this RFC after discovering the complexity of BrowserTabList/TabActor/AddonList. I feel that it isn't really a matter of big files, but rather that this is a particular code, with particularly abstracted patterns

It's an overall feeling, not focused on webbrowser.js. Having to keep a mental map of which file contains which implementation has made me avoid this part of the code.

it may introduce confusions if you expect all modules from actors/ directory to be actors

About that, I wanted to make it less confusing by suffixing all the actor files with "Actor" so that you easily see what is an actor and what is not.

But it still spawn a new Sandbox, setup a new custom global object and eventually do a Filesystem request.

I don't expect the number of modules to explode anyway, but I should do some benchmarks to get a better idea of the perf impact.

Otherwise I wouldn't make a rule of 1file=1class, but rather only split the big modules like webbrowser and inspector.

I agree it should be done progressively starting with the main offenders. For instance I don't think I'd push for splitting https://searchfox.org/mozilla-central/source/devtools/server/actors/actor-registry.js anytime soon :)

[juliandescottes]

From the current comments in this RFC I think the overall feeling is that it would be OK to split some of the files, while avoiding the extreme "refactor everything". Performance impact should still be assessed quickly before diving into this.

[gregtatum]

From the current comments in this RFC I think the overall feeling is that it would be OK to split some of the files, while avoiding the extreme "refactor everything". Performance impact should still be assessed quickly before diving into this.

This sounds really reasonable and I approve! Also thanks for the feedback @ochameau

[juliandescottes]

Accepted, I will open a meta bug to start tracking this work and the first step will be do a performance assessment and to get a shortlist of files that would be good candidates for a split.

[juliandescottes]

Opened meta at https://bugzilla.mozilla.org/show_bug.cgi?id=1432780