Add DMD option to |mach run|

RESOLVED FIXED in mozilla35

Status

()

Core
DMD
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

It would be nice if we could specify that we want DMD enabled via the |mach run| command rather than having to set several env vars manually.

Ideal usage would be:
    |./mach run --dmd|
It is a nice idea. The only tricky part is working out a nice way to specify additional arguments, such as --sample-below.
Created attachment 8479418 [details] [diff] [review]
Add dmd command to |mach|

This adds a new |mach dmd| command. Without additional arguments it launches with DMD enabled using default options. |sample_below|, |max_frames|, and |max_records| may all be overriden via command-line params.

Nick, let me know if this is works for you, also if you could test on linux that would be helpful.
Attachment #8479418 - Flags: feedback?(n.nethercote)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8479418 [details] [diff] [review]
Add dmd command to |mach|

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

f+ for the general idea.

I don't like the amount of duplication from the existing |mach run| command. A mach expert (gps?) might be able to suggest how to avoid that.

Also, I realize that the '+'-prefixed options are already present in |mach run|, but I think they're awful and shouldn't be emulated.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +900,5 @@
> +        help='Do not pass the -no-remote argument by default.')
> +    @CommandArgument('+background', '+b', action='store_true',
> +        help='Do not pass the -foreground argument by default on Mac')
> +    @CommandArgument('+profile', '+P', action='store_true',
> +        help='Specify the profile to use')

Missing a period (I realize the code you copied had the same problem).

@@ +904,5 @@
> +        help='Specify the profile to use')
> +    @CommandArgument('+sample_below', default=None, type=str,
> +        help='The sample size to use, [1..n]. Default is 4093.')
> +    @CommandArgument('+max_frames', default=None, type=str,
> +        help='The max amount of stack frames to capture in allocation traces, [1..24] Default is 24.')

s/amount/number/

@@ +906,5 @@
> +        help='The sample size to use, [1..n]. Default is 4093.')
> +    @CommandArgument('+max_frames', default=None, type=str,
> +        help='The max amount of stack frames to capture in allocation traces, [1..24] Default is 24.')
> +    @CommandArgument('+max_records', default=None, type=str,
> +        help='Amount of stack trace records to print of each kind, [1..1000000]. Default is 1000.')

s/Amount/Number/
Attachment #8479418 - Flags: feedback?(n.nethercote) → feedback+
My vote for enabling DMD-specific options is:

  mach run --dmd # runs with DMD
  mach run --dmd='--sample-below=N --other-things=...' # runs with DMD with custom options
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Also, I realize that the '+'-prefixed options are already present in |mach
> run|, but I think they're awful and shouldn't be emulated.

I attempted to use --blah but mach failed to run with only cryptic output about "not enough params" or something to that effect.

(In reply to Nathan Froyd (:froydnj) from comment #4)
> My vote for enabling DMD-specific options is:
> 
>   mach run --dmd # runs with DMD
>   mach run --dmd='--sample-below=N --other-things=...' # runs with DMD with
> custom options

I somewhat agree with this, although being able to break out the options and document them individually is kind of nice. In fact I attempted to do this, unfortunately there doesn't appear to be a way for me to specify that a flag can be a bool or a str. AFAICT mach requires that a flag of the type string must have a value provided.
Created attachment 8491569 [details] [diff] [review]
Add ability to launch with DMD enabled with |mach dmd|
Attachment #8491569 - Flags: review?(n.nethercote)
Attachment #8479418 - Attachment is obsolete: true
This switches over to '--params' and consolidates the shared parsing logic between |mach run| and |mach dmd|.

Usage:
  |mach dmd|
  |mach dmd --max_frames 10 https://mozilla.org|
  |mach dmd --sample_below 1 -- -jsconsole -P 'Tester'|
Comment on attachment 8491569 [details] [diff] [review]
Add ability to launch with DMD enabled with |mach dmd|

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

Looks good to me, but I think a mach expert should review as well. Thanks.
Attachment #8491569 - Flags: review?(n.nethercote)
Attachment #8491569 - Flags: review?(mshal)
Attachment #8491569 - Flags: review+
Comment on attachment 8491569 [details] [diff] [review]
Add ability to launch with DMD enabled with |mach dmd|

I'm not a mach expert, but I checked with a few others in #build and it sounded like there weren't any major objections to adding the 'dmd' command. If gps has concerns or a better solution after he gets back, we can always address them with a follow-up.
Attachment #8491569 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/2b69b1ccded0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.