Allow --disable-rust-debug if --enable-debug is set

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla56

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

--enable-debug implies --enable-rust-debug, which has a significant impact on performance for Rust code. While this is often acceptable, there are also times when it is very annoying. Specifically when using it with WebRender, the rendering performance can be abysmal under certain loads. On Linux it is possible with aggressive scrolling to get the browser into a locked up state because the rendering always behind vsync. Obviously we would want to avoid that, but for developers who only care about C++ assertions for their local builds, we shouldn't make them suffer in the meantime and in case of similar future scenarios :).

I propose adding --enable-cpp-debug which sets MOZ_DEBUG like --enable-debug does now, and making it implied by --enable-debug (so the default remains the same as it does now).
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Why not make --enable-debug --disable-rust-debug work, instead?
Also, --enable-cpp-debug implies it'd be limited to C++, where that's not actually the case. There are plenty of non-C++ stuff that use MOZ_DEBUG.
Comment on attachment 8874417 [details] [diff] [review]
Add --enable-cpp-debug to build (only) C++ code with assertions., v1

Review of attachment 8874417 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that this would be useful, but it sounds like a little refinement is needed, given glandium's comments.
Attachment #8874417 - Flags: review?(nfroyd)
glandium, do you mean something like this?
Attachment #8874417 - Attachment is obsolete: true
Attachment #8875400 - Flags: review?(mh+mozilla)
Comment on attachment 8875400 [details] [diff] [review]
Allow implied defaults to be overridden, v1

Now I see this is dumb, I think I figured out what you meant :). Next patch forthcoming.
Attachment #8875400 - Attachment is obsolete: true
Attachment #8875400 - Flags: review?(mh+mozilla)
Summary: Split --enable-cpp-debug from --enable-debug → Allow --disable-rust-debug if --enable-debug is set
Ugh turn off C++ brain, no explicit return None required.
Attachment #8876277 - Attachment is obsolete: true
Attachment #8876277 - Flags: review?(nfroyd)
Attachment #8876278 - Flags: review?(nfroyd)
Comment on attachment 8876278 [details] [diff] [review]
0001-Bug-1370209-Allow-setting-disable-rust-debug-when-en.patch

Review of attachment 8876278 [details] [diff] [review]:
-----------------------------------------------------------------

I think the patch makes sense, but have you tested it with all the combinations of --enable/disable-rust-debug and --enable/disable-debug (and maybe --enable/disable-optimize) to ensure that Rust code gets compiled the way you want?

r=me, assuming we have appropriate testing done per above.
Attachment #8876278 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] (out until 24 July) from comment #9)
> Comment on attachment 8876278 [details] [diff] [review]
> 0001-Bug-1370209-Allow-setting-disable-rust-debug-when-en.patch
> 
> Review of attachment 8876278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the patch makes sense, but have you tested it with all the
> combinations of --enable/disable-rust-debug and --enable/disable-debug (and
> maybe --enable/disable-optimize) to ensure that Rust code gets compiled the
> way you want?
> 
> r=me, assuming we have appropriate testing done per above.

Today I got around to doing this. Built with all the combinations of the above flags, produced the appropriate MOZ_RUST_DEBUG flags and release vs debug rust crates.
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f61b0a0397e
Allow setting --disable-rust-debug when --enable-debug is used. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/9f61b0a0397e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.