820401 - Default DMD to sample-below=4093

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Justin Lebar (not reading bugmail)]

We concluded that sample-below 4099 beats sample-below 4096.  I presume 4093 would perform about the same as 4099, except we'll record 4096-byte allocs precisely, although we'd of course want to test that.

Anyway, whatever default we think is best, we should expose that as a default.  People shouldn't have to remember --sample-below=4093.

I propose we call this arg "--sample", or maybe "--sample-default".

Comment 1

[Justin Lebar (not reading bugmail)]

Actually, I think we should make sampling the default.  Let's make this as easy as possible.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Also print out the sample size, since it's no longer explicit in the $DMD envvar.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 690942 [details] [diff] [review]
Patch, v1

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

Heh, I was just thinking about asking you if you thought this was a good idea :)

At the top of RunTestMode(), just after the |Writer| declaration, please set |gSampleBelowSize| to 1 and add a comment "The first part of the test requires sampling to be off."

In general, if you're going to be modifying the output, you'll need to be testing with --mode=test.  That might require me making it more robust... it currently fails for me on Mac because a couple of unexpected system lib mallocs occur.  I filed bug 820566 for this.

::: memory/replace/dmd/DMD.cpp
@@ +1754,5 @@
>    size_t totalUsableSize = unreportedUsableSize + reportedUsableSize;
>  
>    WriteTitle("Invocation\n");
> +  W("$DMD = '%s'\n", gDMDEnvVar);
> +  W("Sample-below size = %lld\n\n", (long long)(gSampleBelowSize));

You'll need to update memory/replace/dmd/test-expected.dmd to include this line, once for each of the four sections.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: test.dmd generated with this pach on B2G

Doesn't look quite right, but the part affected by this patch is fine.  :)

Comment 5

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2586f85a81

Comment 6

[Nicholas Nethercote [inactive]]

> Doesn't look quite right, but the part affected by this patch is fine.  :)

Um...

Comment 7

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Um...

I'm not quite sure what that means.  :)

I can't say I'm terribly concerned that the test doesn't work right on B2G given that DMD itself seems to work fine.

Comment 8

[Nicholas Nethercote [inactive]]

It's just weird because the test is doing pretty basic stuff, and the output is not even remotely right -- it's like it's failing to intercept every call to malloc.  I wonder if the fact that the malloc calls are coming from within the same file causes them to be missed on B2G.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6d2586f85a81

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 690942 [details] [diff] [review]
Patch, v1

[Triage Comment]
DMD is npotb.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6733372df4c1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1727b5d27dc3