Closed Bug 820401 Opened 8 years ago Closed 8 years ago

Default DMD to sample-below=4093

Categories

(Core :: DMD, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(2 files)

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".
Actually, I think we should make sampling the default.  Let's make this as easy as possible.
Summary: Add arg to DMD which sets us up to sample-below ~ 4096 → Default DMD to sample size ~4096
Attached patch Patch, v1Splinter Review
Also print out the sample size, since it's no longer explicit in the $DMD envvar.
Attachment #690942 - Flags: review?(n.nethercote)
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.
Attachment #690942 - Flags: review?(n.nethercote) → review+
Doesn't look quite right, but the part affected by this patch is fine.  :)
Assignee: nobody → justin.lebar+bug
Summary: Default DMD to sample size ~4096 → Default DMD to sample-below=4093
> Doesn't look quite right, but the part affected by this patch is fine.  :)

Um...
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/6d2586f85a81
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 690942 [details] [diff] [review]
Patch, v1

[Triage Comment]
DMD is npotb.
Attachment #690942 - Flags: approval-mozilla-beta+
Attachment #690942 - Flags: approval-mozilla-aurora+
Attachment #690942 - Flags: approval-mozilla-beta+ → approval-mozilla-b2g18+
Component: General → DMD
You need to log in before you can comment on or make changes to this bug.