Minor refactoring of DMD to make it easier to add new modes

RESOLVED FIXED in Firefox 41

Status

()

Core
DMD
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8615560 [details] [diff] [review]
part 1 - Define a new function to convert the mode to a string.

These are all kind of a matter of taste, but hopefully you agree with me.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4127a33255d9

The try run hasn't finished, but these changes are all pretty simple so hopefully it will be green.

Encapsulating the mode-string conversion into the options is much nicer in my opinion.
Attachment #8615560 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

3 years ago
Created attachment 8615562 [details] [diff] [review]
part 2 - Make mode checking more generic in dmd.py.

On entry, we establish that the mode must be one of a few possibilities. The only case that ever matters is whether it is dark-matter or not. This change lets you add a new mode by modifying only a couple of lines of dmd.py. The extra checking keeps the strong invariant that we have only one of the known modes. The "if not" format is odd, but it didn't seem worth the effort to flip all of the branches.
Attachment #8615562 - Flags: review?(n.nethercote)
(Assignee)

Comment 3

3 years ago
Created attachment 8615563 [details] [diff] [review]
part 3 - Make the liveOrCumulative variables more generic.

There are a number of liveOrCumulative* variables and keys used, but that name isn't accurate any more if you add a third mode. Renaming to some kind of liveOrCumulativeOrWhatever format every time you add a new mode isn't going to scale well. Instead just give up on the non-specific specificity and just say, hey, these are heapRecords or heapBlocks or whatever.
Attachment #8615563 - Flags: review?(n.nethercote)
(Assignee)

Comment 4

3 years ago
See the patch in bug 1058178 for an example of how few changes it requires with these patches to add a new mode.
(Assignee)

Comment 5

3 years ago
Created attachment 8615566 [details] [diff] [review]
part 2 - Remove redundant assertion for dark matter mode.

This is an unrelated thing I noticed. It seems silly to check for a condition and then immediately assert against it.
Attachment #8615566 - Flags: review?(n.nethercote)
Attachment #8615560 - Flags: review?(n.nethercote) → review+
Attachment #8615566 - Flags: review?(n.nethercote) → review+
Comment on attachment 8615562 [details] [diff] [review]
part 2 - Make mode checking more generic in dmd.py.

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

As discussed on IRC, we can avoid this and instead just change the mode from "contents" to "live" at the very start of dmd.py.
Attachment #8615562 - Flags: review?(n.nethercote)
Attachment #8615563 - Flags: review?(n.nethercote)
(Assignee)

Updated

3 years ago
Attachment #8615562 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8615563 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8615566 - Attachment description: part 4 - Remove redundant assertion for dark matter mode. → part 2 - Remove redundant assertion for dark matter mode.
https://hg.mozilla.org/mozilla-central/rev/0c0b129ab295
https://hg.mozilla.org/mozilla-central/rev/860fcc17b26b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.