827523 - DMD: add some options that are useful when adding new memory reporters

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Nicholas Nethercote [inactive]]

When adding new memory reporters, it's useful to see all the records, not just the top 1000, so you can see exactly which blocks are being reported.  But to prevent the output being overly long, it's also useful to reduce the size of the stack traces.

This bug is about adding options for both of these things.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - DMD: Remove valloc() from the test.

This removes valloc() from the test, because Windows doesn't support it.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - DMD: Swap the diff order in test mode.

This swaps the diff order in test mode so the expected output is first, which
is more intuitive.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - DMD: Add --max-frames and --max-records options.

This patch:
- Adds new --max-frames and --max-records options.
- Bundles all the options in a new |Options| class, which makes things neater.
- Fixes the indentation on the usage message.

Comment 4

[Justin Lebar (not reading bugmail)]

> //---------------------------------------------------------------------------
>+// Options
>+//---------------------------------------------------------------------------
>+
>+struct Options
>+{
>+  template <typename T>
>+  struct NumOption
>+  {
>+    const T mDefault;
>+    const T mMax;
>+    T       mActual;
>+    NumOption(T aDefault, T aMax)
>+      : mDefault(aDefault), mMax(aMax), mActual(aDefault)
>+    {}
>+  };
>+
>+  enum Mode {
>+    Normal,   // run normally
>+    Test,     // do some basic correctness tests
>+    Stress    // do some performance stress tests
>+  };
>+
>+  NumOption<size_t>   mSampleBelowSize;
>+  NumOption<uint32_t> mMaxFrames;
>+  NumOption<uint32_t> mMaxRecords;
>+  Mode mMode;
>+
>+  Options();
>+};

I like the idea of having an Options object, but this API makes it
significantly more verbose to access an option.

How about if we had something like

  struct Options
  {
    enum Mode {
      [...]
    };

    size_t sampleBelowSize;
    uint32_t maxFrames;
    uint32_t maxRecords;
    Mode mode;

    Options(); // sets everything to defaults

    void ParseOptions(int argc, const char** argv)
    {
      [...]
    }
  };

  Options gOptions;  // note, not a pointer

I agree this isn't as nice wrt having the options' [max, default, actual] all
in one object.  But the consumer's API is nicer, and having ParseOptions be
part of Options is better OOP.

Making gOptions not be a pointer is a premature optimization for speed; the
compiler can statically address the members of a gOptions stored in the data
section.  It also saves you one character ("." versus "->").  :)

What do you think?

Comment 5

[Nicholas Nethercote [inactive]]

> I agree this isn't as nice wrt having the options' [max, default, actual] all
> in one object.  But the consumer's API is nicer, and having ParseOptions be
> part of Options is better OOP.

The mMax and mDefault fields are only used for parsing and the usage message.  I'll try to encapsulate those operations inside the class to avoid exposing them.


> Making gOptions not be a pointer is a premature optimization for speed; the
> compiler can statically address the members of a gOptions stored in the data
> section.  It also saves you one character ("." versus "->").  :)

That's not safe.  The comment at the top of Init() says:

// WARNING: this function runs *very* early -- before all static initializers
// have run.  For this reason, non-scalar globals such as gStateLock and
// gStackTraceTable are allocated dynamically (so we can guarantee their
// construction in this function) rather than statically.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - DMD: Add --max-frames and --max-records options.

New OOPier version.

Comment 7

[Justin Lebar (not reading bugmail)]

> That's not safe.

Okay, but we don't have to rely on any static initializers -- we can make the constructor be a nop and rely on the .DATA section being 0.

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 699470 [details] [diff] [review]
(part 3) - DMD: Add --max-frames and --max-records options.

>@@ -272,20 +265,73 @@ static const size_t kBufLen = 64;
> //---------------------------------------------------------------------------
>+// Options (I)
>+//---------------------------------------------------------------------------

Nit: Could you say "Part I" or something?

>-static bool
>-OptionLong(const char* aArg, const char* aOptionName, long aMin, long aMax,
>-           long* aN)
>+bool
>+Options::Long(const char* aArg, const char* aOptionName, long aMin, long aMax,
>+              long* aN)

Nit: ExtractLong or something?

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9035cb5c0812
https://hg.mozilla.org/integration/mozilla-inbound/rev/b954323bf860
https://hg.mozilla.org/integration/mozilla-inbound/rev/22d57a4edea5

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9035cb5c0812
https://hg.mozilla.org/mozilla-central/rev/b954323bf860
https://hg.mozilla.org/mozilla-central/rev/22d57a4edea5