1058851 - Add DMD option to |mach run|

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Eric Rahm [:erahm]]

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|

Comment 1

[Nicholas Nethercote [inactive]]

It is a nice idea. The only tricky part is working out a nice way to specify additional arguments, such as --sample-below.

Comment 2

[Eric Rahm [:erahm]]

Attachment: 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.

Comment 3

[Nicholas Nethercote [inactive]]

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/

Comment 4

[Nathan Froyd [:froydnj]]

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

Comment 5

[Eric Rahm [:erahm]]

(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.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Add ability to launch with DMD enabled with |mach dmd|

Comment 7

[Eric Rahm [:erahm]]

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 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Michael Shal [:mshal]]

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.

Comment 10

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b69b1ccded0

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b69b1ccded0