Default DMD to sample-below=4093

RESOLVED FIXED in Firefox 19

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

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

Tracking

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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".
(Assignee)

Comment 1

6 years ago
Actually, I think we should make sampling the default.  Let's make this as easy as possible.
(Assignee)

Updated

6 years ago
Summary: Add arg to DMD which sets us up to sample-below ~ 4096 → Default DMD to sample size ~4096
(Assignee)

Comment 2

6 years ago
Created attachment 690942 [details] [diff] [review]
Patch, v1

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+
(Assignee)

Comment 4

6 years ago
Created attachment 691105 [details]
test.dmd generated with this pach on B2G

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

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

6 years ago
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...
(Assignee)

Comment 7

6 years ago
(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.

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/6d2586f85a81
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 10

6 years ago
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+
(Assignee)

Updated

6 years ago
Attachment #690942 - Flags: approval-mozilla-beta+ → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6733372df4c1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1727b5d27dc3
status-b2g18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Component: General → DMD
You need to log in before you can comment on or make changes to this bug.