1603167 - Restructure Sync.jsm and Error.jsm to better deal with lazy loading

Status: NEW

(Remote Protocol :: Agent, task, P3)

Description

[Andreas Tolfsen ❲:ato❳]

Sync.jsm and Error.jsm export a lot of symbols. This is not in itself a bad thing, considering ChromeUtils.import and the forthcoming ES modules import() statement supports it.

But it does not play nicely with XPCOMUtils.defineLazyGetters as outlined in bug 1601618.

Sync.jsm I think is easy: we should create a subfolder remote/sync/ and put each of the synchronisation primitives as separate classes within the subfolder.

I’m more conflicted about Error.jsm because of the number of files it would generate, but I wouldn’t be opposed to imports such as chrome://remote/content/error/UnsupportedError.jsm.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Why do you think that we should split those JSMs into individual ones? They are kinda small, and I don't see a reason why we would have to deal with such a complexity.

Comment 2

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

Why do you think that we should split those JSMs into individual ones? They are kinda small, and I don't see a reason why we would have to deal with such a complexity.

Maybe I misinterpreted your earlier comment that caused me to file this bug:

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from bug 1601618 comment #3)

In those cases it would be good if our modules would just export a single namespace. Very good example is OS. Note that we should do as much as possible lazy loading of modules for JSWindowActor child instances. Otherwise memory usage will spike up, and performance will be degraded.

One way to make it easier to import modules that export multiple symbols is to make a module export only a single symbol.

ochameau has been a strong proponent of this approach in the past and had some good reasons for it that I’ve now forgot. My impression is that the one-class-per-module organisation is pretty consistent throughout DevTools? Could you please enlighten us?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This also applies to various other global modules like OS (OS.File, OS.Path), NetUtils etc.. As such I would really see following that style.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b153a8919e940f8fc94adb3590998664d35c15d

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #2)

ochameau has been a strong proponent of this approach in the past and had some good reasons for it that I’ve now forgot. My impression is that the one-class-per-module organisation is pretty consistent throughout DevTools? Could you please enlighten us?

It helps knowing from which JSM a given class comes from.
If you have a class called Foo, you will look for Foo.jsm.
I think that's the first reason for one-class-per-module.
A second reason would be lazy loading.
If you only use one of the many classes, you can load less JS and loading any byte of JS is an expensive operation.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1603167 - [remote] Restructure Sync.jsm for single identifier import.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1603167 - [remote] Restructure Error.jsm for single identifier import.

Depends on D58813

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Based on the discussion in the Phabricator revisions I'm going to stop work for now.