If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Status

()

Core
DMD
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla36
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Three DMD tweaks coming up.
(Assignee)

Comment 1

3 years ago
Created attachment 8508357 [details] [diff] [review]
(part 1) - DMD: make some very short functions one-liners
Attachment #8508357 - Flags: review?(continuation)
Comment on attachment 8508357 [details] [diff] [review]
(part 1) - DMD: make some very short functions one-liners

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

::: memory/replace/dmd/DMD.cpp
@@ +323,3 @@
>  
> +  void Lock()   { EnterCriticalSection(&mCS); }
> +  void Unlock() { LeaveCriticalSection(&mCS);

nit: looks like an extra new line in here
Attachment #8508357 - Flags: review?(continuation) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8508360 [details] [diff] [review]
(part 2) - Remove dmd.py's -b option and make its behaviour the default

dmd.py's -b option (a.k.a. --show-all-block-sizes) is really useful. For
example, in a record like this:

  426 blocks in heap block record 1 of 14,442
  27,918,336 bytes (27,918,336 requested / 0 slop)
  6.22% of the heap (6.22% cumulative)

I used to regularly fire up python to compute |27918336 / 426.0| in order to
see if all the blocks are the same size. But with -b I get this extra line:

  Individual block sizes: 65,536 x 426

In fact, it's so useful that I think it should always be on, and there's no
need to be able to disable it. I.e. the option can be removed.

This patch removes -b and makes that behaviour the default, with the following
tweaks:

- It now puts that line before the "of the heap" line, instead of after it.
  
- It doesn't show that line if the record contains a single block, because the
  list is redundant in that case.
Attachment #8508360 - Flags: review?(continuation)
(Assignee)

Comment 4

3 years ago
Created attachment 8508363 [details] [diff] [review]
(part 3) - Print dmd.py's invocation at the top of its output

This is quite useful. I will characterise the use of basename as a "pragmatic
hack".
Attachment #8508363 - Flags: review?(continuation)
Comment on attachment 8508360 [details] [diff] [review]
(part 2) - Remove dmd.py's -b option and make its behaviour the default

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

Neat.

My only comment would be to see how long the list can get in a typical file, and consider filing a followup to truncate the list if it is long.  When I've looked at the output of something like this before, you can end up with really huge lists.  Though maybe the consolidation of multiple blocks of the same size is enough to avoid that.
Attachment #8508360 - Flags: review?(continuation) → review+
Attachment #8508363 - Flags: review?(continuation) → review+
(Assignee)

Comment 6

3 years ago
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8508360 [details] [diff] [review]
> (part 2) - Remove dmd.py's -b option and make its behaviour the default
> 
> Review of attachment 8508360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Neat.
> 
> My only comment would be to see how long the list can get in a typical file,
> and consider filing a followup to truncate the list if it is long.

I considered it. I looked at output for some large runs and this was the biggest one I saw:

> Individual block sizes: 1,142,784 x 6; 1,007,616; 978,944; 925,696; 798,720; 4 42,368; 405,504 x 2; 401,408; 397,312 x 2; 393,216; 389,120 x 2; 385,024; 380,92 8 x 2; 376,832; 372,736 x 2; 368,640 x 2; 364,544 x 2; 360,448 x 2; 356,352 x 2; 352,256; 348,160 x 2; 344,064; 339,968 x 2; 335,872 x 2; 331,776 x 2; 327,680 x 2; 323,584 x 2; 319,488; 315,392 x 2; 311,296; 307,200; 303,104 x 2; 299,008 x 2; 294,912 x 2; 290,816 x 2; 286,720; 282,624 x 4; 278,528; 274,432 x 4; 270,336 x 2; 266,240 x 2; 262,144 x 4; 258,048 x 3; 253,952 x 3; 249,856 x 2; 245,760 x 5; 241,664 x 3; 237,568 x 4; 233,472 x 2; 229,376 x 4; 225,280; 221,184 x 4; 21 7,088 x 3; 212,992 x 6; 208,896 x 2; 204,800 x 5; 200,704 x 2; 196,608 x 5; 192, 512 x 4; 188,416 x 4; 184,320 x 4; 180,224 x 4; 176,128 x 4; 172,032 x 6; 167,93 6 x 4; 163,840 x 5; 159,744 x 4; 155,648 x 3; 151,552 x 4; 147,456 x 3; 143,360 x 4; 139,264 x 4; 135,168 x 4; 131,072 x 22; 126,976 x 5; 122,880 x 4; 118,784 x 5; 114,688 x 6; 110,592 x 5; 106,496 x 3; 102,400 x 4; 98,304 x 6; 94,208 x 16; 90,112 x 2; 86,016 x 24; 81,920 x 15; 77,824 x 6; 73,728 x 7; 69,632 x 18; 65,5 36 x 142; 61,440 x 20; 57,344 x 11; 53,248 x 15; 49,152 x 54; 45,056 x 56; 40,96 0 x 11; 36,864 x 63; 32,768 x 158; 28,672 x 28; 24,576 x 31; 20,480 x 71; 16,384 x 122; 12,288 x 120; 8,192 x 231; 4,096 x 827; ~4,093 x 7,494

That's from the record for nsStringBuffer::Alloc() in a run with --max-frames=1, which is the worst case. Fairly big, but still less than a page, largely because the granularity of jemalloc's buckets is relatively coarse. So I didn't bother with anything fancy.
(Assignee)

Comment 7

3 years ago
Thank you for the fast reviews.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> That's from the record for nsStringBuffer::Alloc() in a run with
> --max-frames=1, which is the worst case. Fairly big, but still less than a
> page, largely because the granularity of jemalloc's buckets is relatively
> coarse. So I didn't bother with anything fancy.

Yeah, that's not bad.  Plus there's so much that is combined due to the sampling.
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/450c187cbc1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cfce41bed5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bb832d0c539
sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=de805196bbc4 since one of this changes caused perma failure on 10.8 Debug Tests like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3275566&repo=mozilla-inbound
(Assignee)

Comment 11

3 years ago
Attempt 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e82193dab58
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6550843ba9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/04015991eea0
https://hg.mozilla.org/mozilla-central/rev/3e82193dab58
https://hg.mozilla.org/mozilla-central/rev/e6550843ba9f
https://hg.mozilla.org/mozilla-central/rev/04015991eea0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.