Julien Wajsberg [:julienw] (PTO -> may 13) Reporter
	Description • 6 years ago

DEBUG_JS_MODULES is enabled by a line in mozconfig:

  ac_add_options --enable-debug-symbols

while DEBUG is enabled by a similar line:

  ac_add_options --enable-debug

DEBUG_JS_MODULES was intended to run some of our code in a "debug" mode but still using a Firefox production build, running much faster. Most of our code that uses DEBUG_JS_MODULES also checks for DEBUG. The direct result is that gecko developers running a DEBUG build without DEBUG_JS_MODULES also gets the same performance penalty.

One direct effect of DEBUG_JS_MODULES is that we load the dev versions of react and other libraries, which are also a lot slower. As said previously, this happens also for simple DEBUG builds.

The DEBUG mode is aimed at quite a different audience (typically platform engineers working on C++ code) from those who want DEBUG_JS_MODULES (typically DevTools engineers working on JS code). While the same person might work in both areas at different times, it is less likely that they are in both areas at once, so we likely don't need them tied together.


The intended work in this bug is:

1. Everywhere we couple DEBUG and DEBUG_JS_MODULES [1], remove the part about DEBUG. (including moz.build!)

2. Add a note in the documentation in [2]

[1] https://searchfox.org/mozilla-central/search?q=DEBUG_JS_MODULES&case=true&regexp=false&path=
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 1 • 6 years ago

Just to clarify...

DEBUG_JS_MODULES is controlled by:

  ac_add_options --enable-debug-js-modules

`--enable-debug-symbols` is not related to this topic.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 2 • 6 years ago

The RFC that resulted in the creation of this bug:
https://github.com/devtools-html/rfcs/issues/36

We use the following code in `devtools/client/shared/vendor/moz.build`:

```
if CONFIG['DEBUG_JS_MODULES'] or CONFIG['MOZ_DEBUG']:
    DevToolsModules(
        'react-dev.js',
        'react-dom-dev.js',
        'react-dom-server-dev.js',
        'react-dom-test-utils-dev.js',
        'react-prop-types-dev.js',
    )
```

Then inside `devtools/client/shared/browser-loader.js` we do this:

```
if (AppConstants.DEBUG || AppConstants.DEBUG_JS_MODULES) {
    dynamicPaths["devtools/client/shared/vendor/react"] =
      "resource://devtools/client/shared/vendor/react-dev";
    dynamicPaths["devtools/client/shared/vendor/react-dom"] =
      "resource://devtools/client/shared/vendor/react-dom-dev";
    dynamicPaths["devtools/client/shared/vendor/react-dom-server"] =
      "resource://devtools/client/shared/vendor/react-dom-server-dev";
    dynamicPaths["devtools/client/shared/vendor/react-prop-types"] =
      "resource://devtools/client/shared/vendor/react-prop-types-dev";
    dynamicPaths["devtools/client/shared/vendor/react-dom-test-utils"] =
      "resource://devtools/client/shared/vendor/react-dom-test-utils-dev";
  }
```

Unfortunately this means that we are loading the dev version of react in DEBUG builds and this is not desirable so we have decided to only load the react dev versions if `ac_add_options --enable-debug-js-modules` is set in .mozconfig.

	Patrick Brosset <:pbro>
	Comment 4 • 6 years ago

Sorry Mike, but I don't feel comfortable reviewing this. I've never touched any of this code. Ryan will be a better reviewer on this.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8958430 [details]
Bug 1440609 - Uncouple DEBUG_JS_MODULES and DEBUG in devtools code

https://reviewboard.mozilla.org/r/227384/#review233220

Thanks for working on this! :) Overall, it looks good.

I think there is one more place to consider:

* DevToolsUtils.assert (https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/shared/DevToolsUtils.js#444)

I'd like to see another round review because I am curious if some people think that one should be left alone.

::: devtools/client/shared/vendor/moz.build:31
(Diff revision 1)
>      'seamless-immutable.js',
>      'WasmDis.js',
>      'WasmParser.js',
>  )
>  
>  # react dev versions are used if either debug mode is enabled,

We should update this comment to remove mentions of "debug mode" here.

	Nick Alexander :nalexander [he/him]
	Comment 6 • 6 years ago

As a build peer, I'm pleased to see folks teasing apart legacy choices based on flags that existed in the past and not on what we actually want.  Thanks for doing this work!

	Julian Descottes [:jdescottes]
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8958430 [details]
Bug 1440609 - Uncouple DEBUG_JS_MODULES and DEBUG in devtools code

https://reviewboard.mozilla.org/r/227384/#review233278

::: devtools/client/jsonview/converter-child.js:17
(Diff revision 1)
>  
>  loader.lazyRequireGetter(this, "NetworkHelper",
>                                 "devtools/shared/webconsole/network-helper");
>  loader.lazyGetter(this, "debug", function() {
>    let {AppConstants} = require("resource://gre/modules/AppConstants.jsm");
> -  return !!(AppConstants.DEBUG || AppConstants.DEBUG_JS_MODULES);
> +  return !!(AppConstants.DEBUG_JS_MODULES);

Wondering if this should not be the DEBUG rather than DEBUG_JS_MODULES.

This flag is used in exportData(). I'm not sure we want to know if we use debug versions of JS libraries, or if we are on a debug build? I'm not even sure the flag is used :)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 8 • 6 years ago
mozreview-review-reply

Comment on attachment 8958430 [details]
Bug 1440609 - Uncouple DEBUG_JS_MODULES and DEBUG in devtools code

https://reviewboard.mozilla.org/r/227384/#review233278

> Wondering if this should not be the DEBUG rather than DEBUG_JS_MODULES.
> 
> This flag is used in exportData(). I'm not sure we want to know if we use debug versions of JS libraries, or if we are on a debug build? I'm not even sure the flag is used :)

It's exported into the JSON Viewer's scope in content, and then referenced here:

https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/client/jsonview/viewer-config.js#33

to choose which modules to use.  So, `DEBUG_JS_MODULES` seems like correct flag here.  (Namely it simply `debug` is certainly a bit mysterious, so a longer name in JSON Viewer might make things more obvious...)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 9 • 6 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> * DevToolsUtils.assert
> (https://searchfox.org/mozilla-central/rev/
> 99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/shared/DevToolsUtils.
> js#444)

Seems like we did not get much of a feeling about this one during the DevTools meeting at least.

Personally, I believe we _do_ want to change this one as well, for the same reason as the others: such assertions are of interest to DevTools team working on the JS side, and are likely _not_ of interest to C++ devs running `DEBUG` builds, so we should unlink the concepts here.

	Julian Descottes [:jdescottes]
	Comment 10 • 6 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> 
> It's exported into the JSON Viewer's scope in content, and then referenced
> here:
> 
> https://searchfox.org/mozilla-central/rev/
> 99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/client/jsonview/viewer-
> config.js#33
> 
> to choose which modules to use.  So, `DEBUG_JS_MODULES` seems like correct
> flag here.  (Namely it simply `debug` is certainly a bit mysterious, so a
> longer name in JSON Viewer might make things more obvious...)

Ah ok! Yes in this case `DEBUG_JS_MODULES` is the right flag, and we could rename the variable/property to debugJsModule to be consistent with the decoupling? 

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > * DevToolsUtils.assert
> > (https://searchfox.org/mozilla-central/rev/
> > 99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/shared/DevToolsUtils.
> > js#444)
> 
> Seems like we did not get much of a feeling about this one during the
> DevTools meeting at least.
> 
> Personally, I believe we _do_ want to change this one as well, for the same
> reason as the others: such assertions are of interest to DevTools team
> working on the JS side, and are likely _not_ of interest to C++ devs running
> `DEBUG` builds, so we should unlink the concepts here.

I agree

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 11 • 6 years ago

DevToolsUtils.assert is **only** used in the following places:

devtools/client/memory/*
devtools/client/performance/*
devtools/client/shared/browser-loader.js
devtools/client/shared/components/HSplitBox.js
devtools/server/actors/*

I also agree that it should be DEBUG_JS_MODULES only.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 13 • 6 years ago
mozreview-review

Comment on attachment 8958430 [details]
Bug 1440609 - Uncouple DEBUG_JS_MODULES and DEBUG in devtools code

https://reviewboard.mozilla.org/r/227384/#review233468

Hmm, I think you meant to also add `assert` changes as well?  Otherwise, it looks good, but I just want to make sure we include each change we intend to have here.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 15 • 6 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> Comment on attachment 8958430 [details]
> Bug 1440609 - Uncouple DEBUG_JS_MODULES and DEBUG in devtools code
> 
> https://reviewboard.mozilla.org/r/227384/#review233468
> 
> Hmm, I think you meant to also add `assert` changes as well?  Otherwise, it
> looks good, but I just want to make sure we include each change we intend to
> have here.

Don't get what happened... it is there in my local copy.

I have pushed to mozreview again after rebasing and can see the assert change so hopefully you can.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 16 • 6 years ago
mozreview-review

Comment on attachment 8958430 [details]
Bug 1440609 - Uncouple DEBUG_JS_MODULES and DEBUG in devtools code

https://reviewboard.mozilla.org/r/227384/#review233704

Thanks, this looks good to me! :)

	Pulsebot
	Comment 17 • 6 years ago

Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fbde23ef617
Uncouple DEBUG_JS_MODULES and DEBUG in devtools code r=jryans

	Stefan Hindli [:stefan_hindli]
	Comment 18 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0fbde23ef617