Per conversation with :kmoir, I'm going through untriaged bugs in her components and marking the ones which look to be enhancements/tasks with the enhancement severity to get them out of the triage queue.

If this incorrect, please remove the tag.

I think this falls under the influence of the JavaScript usage module so needinfoing the rest of them for feedback.

My chief concerns with doing this are three-fold. Firstly there is the increased build time, secondly there is the loss of good error messages in the console and thirdly debugging minified code is painful. We did a lot of work in years past to avoid any preprocessing of JS precisely for all those reasons. We have explicitly ignored using potentially useful tools at build time such as JSX and TypeScript for the same.

Source maps solve much of the problems around debugging but comment 4 made the statement that we wouldn't ship source maps in the build and I think that makes sense. What I don't know if there are sensible ways we could host the source maps online such that our devtools can make use of them when needed?

Without source maps we could mitigate some of the issues here by not minifying for local builds, so there would be no impact to local development. We could even potentially not minify in nightly builds. That increases the risk of the minification itself introducing a change in behaviour but given how popular these tools are in the wild that seems unlikely.

We should also have an estimate of how much this would save, so we can weight pros and cons.

We also still have a lot of JS calls in markup files, and we use a lot of stuff esp. in browser.xhtml where functions in a.js just depend on stuff declared in b.js and c.js existing in the same scope, so we'd have to do a bunch of work to figure out what function names are implicitly exported and/or coordinate between the different files, and/or switch some of those files to "proper" modules where that makes sense.

A rough idea of how much memory is taken up by JS loaded in the parent and/or content processes at this point, and how it compares if we minified it, would indeed help estimate benefits from this approach as noted in comment #10.

I agree that I wouldn't want to ship this unless we had good devtools integration, where opening the browser toolbox would automatically go get source maps for anything displayed there (for release/beta builds etc.). I don't know if this exists today and/or if it's straightforward to build - maybe Julian can help clear that up?

I think most of my concerns are already covered by Dave & Gijs. For me the developer experience is the significant part here, we need to be approximately the same so that we keep the same momentum.

It may also be worth stating explicitly - when we get bug reports from users they are frequently asked to report what's on the error console as well. We need to be able to easily interpret those, hence one of the reasons we'll need source maps on beta & release.

Note that bug 903149 contains lots of useful information when FirefoxOS was minified:

• existing minifiers may not support gecko-specific syntax, or very latest JS proposals eagerly used in tree. This may slow us down in using very latest work from spidermonkey.
• the minifiers experimented in that bug failed to minify a few files.
• only stripping comments and blank lines brings most of the possible improvements (see early comments, unfortunately, fabrice experiments were not generating source-map). This is something which we may do first to prevent introducing any bug with more complex minification and wouldn't loose too much information in case we have sub-obtimal source-map support.

I'm personaly on the fence of doing build time JS transformation. Here is why.
The debugger has been doing this for years now. The historical reason was to use ES6 features before they got implemented in Gecko (ES Classes, privates fields, ES Modules). It is still using a build step for JSX but that's something I would like to get rid of...
I'm actually the one having introducing this build step in mach via bug 1461714, to prevent having to manually run npm/yarn to update the debugger.
Since then, we never had time to setup a source-map environment just for the debugger and experienced all the cons mentioned in this bug.

But I'm wondering why we are still parsing chrome JS files?
Per bug 903149 comment 41 and 42, don't we ship pre-compiled JS binaries in the omni.jar?
Is it only done for a subset of files?
Shouldn't we rather investigate doing this as it would probably be faster than the most advanced minifier.
If we do pre-compile all files, why minifying beforehand brings such improvements, isn't there an issue with spaces/comments which we could fix?
Does that mean pre-compiled JS binaries are storing the whole JS source? If so it sounds like something only useful when devtools or the famous toSource are used and may be optimized in some way.

(In reply to Dave Townsend [:mossop] from comment #9)

I think this falls under the influence of the JavaScript usage module so needinfoing the rest of them for feedback.

My chief concerns with doing this are three-fold. Firstly there is the increased build time, secondly there is the loss of good error messages in the console and thirdly debugging minified code is painful. We did a lot of work in years past to avoid any preprocessing of JS precisely for all those reasons. We have explicitly ignored using potentially useful tools at build time such as JSX and TypeScript for the same.

Taking my hat of devil's advocate: see comment 6, which mentions the existing minification step being part of the packaging step instead of build.
If we keep this setup, typical developer workflow would stay the same.

Without source maps we could mitigate some of the issues here by not minifying for local builds, so there would be no impact to local development. We could even potentially not minify in nightly builds. That increases the risk of the minification itself introducing a change in behaviour but given how popular these tools are in the wild that seems unlikely.

This, or doing only on the packaging step would significantly reduce mozilla-central developers hability to uncover minification issues. Bug 903149 highlighted that there were a few!
Also, the usage of the source-map setup will be significantly reduced. I suspect we wouldn't put as much effort in making it fast and reliable as if we were enabling minification on the default build step.

(In reply to :Gijs (he/him) from comment #11)

I agree that I wouldn't want to ship this unless we had good devtools integration, where opening the browser toolbox would automatically go get source maps for anything displayed there (for release/beta builds etc.). I don't know if this exists today and/or if it's straightforward to build - maybe Julian can help clear that up?

Me and :bomsy are probably the most involved with debugger and source-map.
I'd be happy to coordinate with anyone experimenting with a minifier generating source-maps to make it work with the browser console and toolbox.
It should work out-of-the-box if the JS file exposes the source-map url in comment and that URL exists.
You might only struggle with a security restriction on the source map URL if the source-map is hosted via resource:// URI:
https://searchfox.org/mozilla-central/rev/fb55a4ee44a9a95d099aa806ca494eb988252ded/devtools/client/shared/source-map-loader/utils/network-request.js#8-17

Note that I suspect we would never support source-map in Console.cpp and JS Error stack objects (because source-map is asynchronous [and slow] which would delay logging and mix up logs ordering), so anything printed to stdout via console APIs or printing JS Error stacks would probably always refer to minified source. This may be a significant issue on try or when running tests with minification enabled.
I recently introduced bug 1825511 which would help opening the minified location from stdout into the debugger, which would then allow you to open the original matching location. But that's clearly sub-optimal compared to seeing the original function names and location right into stdtout.

Clearing my need info, thanks Alex for providing feedback from the DevTools side in much more details than I could.

The only thing I will add is that all technical issues set aside, sourcemaps support in DevTools is not on par with the regular debugging experience yet. If we go for this we can expect more bugs and issues at least in the beginning. However the plus side of that is that this will lead us to improve our sourcemaps support which should also benefit web developers in the end.

Some thoughts from the SpiderMonkey side:

(1) We've removed most/all of the SpiderMonkey-specific JS syntax, so I don't expect major issues there nowadays.

(2) As mentioned by others, support for newer language features could be an issue. I'd expect the popular minifiers to add support for these fairly quickly though. Worth double checking.

(3) SpiderMonkey has various ways to mitigate memory overhead for in-memory JS source code:

• Source code in memory is compressed with zlib (after a short delay) and decompressed when required for lazy parsing or function.toString().
• We have a parse option to discard source code entirely after parsing. See setDiscardSource. This does prevent syntax-only parsing and relazification (discarding bytecode) for functions and this results in more bytecode being generated, so it's a trade-off. (It also affects toString in non-standard ways but that's fine for chrome code we control.)
• We also support "lazy source code". In this case we use a callback when source code is required for function.toString(). See setSourceIsLazy and XPCJSSourceHook. This currently also disables the syntax parser so also trades source code for bytecode.

(4) Minifying names of local variables etc could be interesting for memory usage and performance because it likely reduces the number of live atoms. I'd be interested in an about:memory comparison.

(In reply to Alexandre Poirot [:ochameau] from comment #13)

But I'm wondering why we are still parsing chrome JS files?
Per bug 903149 comment 41 and 42, don't we ship pre-compiled JS binaries in the omni.jar?
Is it only done for a subset of files?

I think we removed this pre-generated startup cache in bug 1351071. A problem with pre-compiled or cached binaries without source code is also that it results in more bytecode being generated because of syntax parsing being disabled, so it's not an obvious memory improvement.

(In reply to Jan de Mooij [:jandem] from comment #15)

(In reply to Alexandre Poirot [:ochameau] from comment #13)

But I'm wondering why we are still parsing chrome JS files?
Per bug 903149 comment 41 and 42, don't we ship pre-compiled JS binaries in the omni.jar?
Is it only done for a subset of files?

I think we removed this pre-generated startup cache in bug 1351071. A problem with pre-compiled or cached binaries without source code is also that it results in more bytecode being generated because of syntax parsing being disabled, so it's not an obvious memory improvement.

Oh I missed that removal.
Isn't it something we could revive now that mozilla-central no longer uses XUL/XBL and even the subscriptloader which is less and less used thanks to the ESM project.
When I see this old discussion about enabling JSBC for chrome:
https://dev-platform.mozilla.narkive.com/rnQd3LOm/jsbc-javascript-start-up-bytecode-cache#post4
Many of the complexities discussed back then no longer exists. It could be done either at build time or at runtime.
I imagine that if JSBC was benefitial to the web... it would be similarly benefitial to chrome JS?

(In reply to Alexandre Poirot [:ochameau] from comment #16)

I think we removed this pre-generated startup cache in bug 1351071. A problem with pre-compiled or cached binaries without source code is also that it results in more bytecode being generated because of syntax parsing being disabled, so it's not an obvious memory improvement.

Oh I missed that removal.
Isn't it something we could revive now that mozilla-central no longer uses XUL/XBL and even the subscriptloader which is less and less used thanks to the ESM project.

We could, and I've been considering doing it for several reasons. It was mainly removed because when we accidentally stopped generating it, we didn't see any impact on performance. But at this point, I've actually been considering the possibility of just shipping pre-compiled JS and not shipping most JS sources at all on Android for the sake of reduced APK size on Android. It would also theoretically help startup perf after upgrades and in the other cases where we purge the startup cache, but I'm not really sure how much. We'd need to measure.

(In reply to Jan de Mooij [:jandem] from comment #15)

Some thoughts from the SpiderMonkey side:

(1) We've removed most/all of the SpiderMonkey-specific JS syntax, so I don't expect major issues there nowadays.

(2) As mentioned by others, support for newer language features could be an issue. I'd expect the popular minifiers to add support for these fairly quickly though. Worth double checking.

(3) SpiderMonkey has various ways to mitigate memory overhead for in-memory JS source code:

Yes, I'd really be surprised at this point if having minified sources would help memory usage at all, since we typically try to avoid storing chrome JS sources to begin with. I think possibly the biggest improvement in memory usage would be increasing the amount of content JS we store cached bytecode for since, aside from removing the need to load and parse the source, it would also mean we would get to store the bytecode for those scripts in shared memory, and therefore not waste memory on the bytecode either.

That is actually another reason I've been considering generating bytecode at build time. It would make it easier to generate a content preloader file with the sources that would likely be needed without several restarts, and in the cases where we don't already have them, we would be able to just read them from omni.ja rather than parsing them from scratch.

But, at the point where we're generating JS bytecode at build time, we could possibly also just store it all in libxul and remove the need for the preloader cache at all.

(4) Minifying names of local variables etc could be interesting for memory usage and performance because it likely reduces the number of live atoms. I'd be interested in an about:memory comparison.

I'd be interested in this too. I'd hope it wouldn't make much difference, but I've also been wanting to use shared memory for atom names that are used by chrome JS for a while too, which would hopefully have an even bigger win.

Yes, I'd really be surprised at this point if having minified sources would help memory usage at all.

I found in a memory regression investigation that just deleting the comments in my TranslationsChild.sys.mjs file reduced memory usage by 4,608 bytes per content process. It might be worth doing a quick experiment of just minifying the child actors and measuring the the awsy results.

(In reply to Greg Tatum [:gregtatum] from comment #18)

Yes, I'd really be surprised at this point if having minified sources would help memory usage at all.

I found in a memory regression investigation that just deleting the comments in my TranslationsChild.sys.mjs file reduced memory usage by 4,608 bytes per content process. It might be worth doing a quick experiment of just minifying the child actors and measuring the the awsy results.

OK. We should look into fixing that by either making sure we're dropping the source for that file after it's compiled, or making sure it's loaded from the preloader cache so we never have to load the source in most content processes to begin with. But either way, that's something that, if it's happening, we already have the tools to fix at the platform level without resorting to minification.

(In reply to Kris Maglione [:kmag] from comment #19)

OK. We should look into fixing that by either making sure we're dropping the source for that file after it's compiled, or making sure it's loaded from the preloader cache so we never have to load the source in most content processes to begin with. But either way, that's something that, if it's happening, we already have the tools to fix at the platform level without resorting to minification.

Can you drive filing the requisite bugs to ensure this work happens?

(In reply to Kris Maglione [:kmag] from comment #19)

OK. We should look into fixing that by either making sure we're dropping the source for that file after it's compiled, or making sure it's loaded from the preloader cache so we never have to load the source in most content processes to begin with. But either way, that's something that, if it's happening, we already have the tools to fix at the platform level without resorting to minification.

If I can summarise it sounds like based on this minification has little in the way of upside for desktop at this point. It makes debugging issues in the wild harder, even if we do the CI work to publish source maps somewhere they don't affect the console and the debugging experience is worse. And we have tools available that should give the same memory usage wins but we're just not making use of them. Unless there is something else to add here this is probably going to turn into a wontfix.

And we could do something to make sure that we are not missing any module (one way would be to compare memory usage between a minified version, which we don't ship, and the default non-minified version, but that's a bit expensive).

(In reply to Dave Townsend [:mossop] from comment #21)

And we have tools available that should give the same memory usage wins but we're just not making use of them.

Either we aren't using them or they aren't tuned properly. It's possible that we do drop the sources, but only after a shrinking GC. That isn't a problem when we load scripts from the startup cache or the script preloader, where we never load them to begin with. But, either way, dropping the sources doesn't give us the same wins as minification. It gives us much bigger wins.

Precompiling the startup cache at build time would make the problem entirely go away, since we would be able to use that cache in the content process for all scripts that we load. It would also be pretty trivial.

Getting better at storing all scripts used in the content process in the script preloader would also make the problem go away, and would give us even bigger wins by allowing us to share bytecode for those scripts as well. But it would take a bit more work, and wouldn't take effect until the first restart after a script is used in a content process.

(In reply to :Gijs (he/him) from comment #20)

Can you drive filing the requisite bugs to ensure this work happens?

Yes. Leaving the NI until I do.