Closed
Bug 1171647
Opened 9 years ago
Closed 9 years ago
Minor refactoring of DMD to make it easier to add new modes
Categories
(Core :: DMD, defect)
Core
DMD
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(2 files, 2 obsolete files)
2.69 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8615560 -
Flags: review?(n.nethercote) → review+
Updated•9 years ago
|
Attachment #8615566 -
Flags: review?(n.nethercote) → review+
Comment 6•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8615563 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•9 years ago
|
Attachment #8615562 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8615563 -
Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0b129ab295 https://hg.mozilla.org/integration/mozilla-inbound/rev/860fcc17b26b
Assignee | ||
Updated•9 years ago
|
Attachment #8615566 -
Attachment description: part 4 - Remove redundant assertion for dark matter mode. → part 2 - Remove redundant assertion for dark matter mode.
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c0b129ab295 https://hg.mozilla.org/mozilla-central/rev/860fcc17b26b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•