Closed
Bug 603606
Opened 14 years ago
Closed 13 years ago
Create a switch to disable DRC by doing nothing during reaping
Categories
(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)
Tamarin Graveyard
Garbage Collection (mmGC)
Tracking
(Not tracked)
RESOLVED
FIXED
Q3 11 - Serrano
People
(Reporter: lhansen, Assigned: treilly)
References
Details
Attachments
(1 file, 4 obsolete files)
5.62 KB,
patch
|
lhansen
:
review+
|
Details | Diff | Splinter Review |
This is an experimental hack for testing: MMGC_DISABLEDRC=1 will disable DRC by simply clearing the ZCT during reaping. A few more adjustments may be required. If this is successful we may promote it to an internal switch in the Flash Player's config files (to get rid of the environment variable), but since it's for specially blessed builds only that may not matter too much.
Reporter | ||
Comment 1•14 years ago
|
||
Passes acceptance testing in Debug and ReleaseDebugger builds of current TR tip.
Assignee | ||
Comment 2•14 years ago
|
||
I would have put the short circuit in Add but this works.
Reporter | ||
Comment 3•14 years ago
|
||
New patch coming up to at least print a debug message once if the switch is properly enabled.
Priority: -- → P3
Target Milestone: --- → flash10.x - Serrano
Reporter | ||
Comment 4•14 years ago
|
||
Changed it as Tommy suggested, added a GCLog message that prints an obvious line of text if DRC is disabled when the ZCT is constructed. (I'm assuming GCLog messages are printed even in Flash Player release builds, I've not verified that.)
Attachment #482559 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Priority: P3 → --
Target Milestone: flash10.x - Serrano → Future
Reporter | ||
Updated•13 years ago
|
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Whiteboard: has-patch
Assignee | ||
Comment 5•13 years ago
|
||
This is similiar to a version I've been using except I put the check in AddSlow and had the ZCT ctor set top= limit to trigger slow path so as not to slow down the inlined Add function. I'll update this patch to do that if you think its worthwhile. Possibly overkill.
Reporter | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > This is similiar to a version I've been using except I put the check in AddSlow > and had the ZCT ctor set top= limit to trigger slow path so as not to slow > down the inlined Add function. I'll update this patch to do that if you > think its worthwhile. Possibly overkill. If it allows us to just land the code (because we're not slowing down any fast path anywhere) then sure...
Reporter | ||
Comment 7•13 years ago
|
||
Requested by Lee and Oliver for data gathering in Serrano builds, and highly desired for the next incubator build. We need to be sure that there is no run-time performance hit when running with DRC as a result of the change (ie it needs to be off on the slow path somewhere, probably in AddSlow as you say). In the player it needs to be surfaced through mm.cfg, that's a separate issue but you should consider it part of this bug.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: Future → Q3 11 - Serrano
Reporter | ||
Updated•13 years ago
|
Whiteboard: has-patch
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → treilly
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #529343 -
Flags: review?(lhansen)
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 529343 [details] [diff] [review] MMGC_DRC env var to disable drc set to 0 Environment variables should not be decoded in the GC code, they should be decoded in the shell code (current precedent notwithstanding) and should not be available in Player builds. In principle wrapping it in #ifdef AVMSHELL_BUILD would do the trick but I don't like that either and would probably R- it as well. The comment for the 'drc' variable is insufficient. It should note that the switch is intended for experimentation and that the performance impact is always negative and usually significantly so, and that some content may break. The isDRCEnabled switch does not belong on the policy manager, all other such switches are available directly on the gc.
Attachment #529343 -
Flags: review?(lhansen) → review-
Assignee | ||
Comment 10•13 years ago
|
||
Makes sense to make it an official cmd line arg and flash cfg file flag if we're gonna be living with this for a couple releases so I just dropped the env var support.
Attachment #529343 -
Attachment is obsolete: true
Attachment #529510 -
Flags: review?(lhansen)
Assignee | ||
Updated•13 years ago
|
Attachment #529510 -
Attachment is obsolete: true
Attachment #529510 -
Flags: review?(lhansen)
Assignee | ||
Comment 11•13 years ago
|
||
Also ditched env var since this will become a standard flag for a couple releases and we'll want to expose as a cfg file switch for flash.
Attachment #483152 -
Attachment is obsolete: true
Attachment #529522 -
Flags: review?(lhansen)
Reporter | ||
Comment 12•13 years ago
|
||
Comment on attachment 529522 [details] [diff] [review] Take 2, cleaned up and also linked up sensibly to nogc mode R- only because the introduction of the drc variable in policy manager persists (though it's never used that way). Feel free to assume R+ after that's been fixed.
Attachment #529522 -
Flags: review?(lhansen) → review-
Assignee | ||
Comment 13•13 years ago
|
||
(In reply to comment #12) > R- only because the introduction of the drc variable in policy manager persists > (though it's never used that way). Feel free to assume R+ after that's been > fixed. I think you are getting tripped up by the fact that GCConfig is defined in GCPolicyManager.h.
Comment 14•13 years ago
|
||
changeset: 6253:1e5dc1cff736 user: Tommy Reilly <treilly@adobe.com> summary: Bug 603606 - Disable DRC switch (r=lhansen) http://hg.mozilla.org/tamarin-redux/rev/1e5dc1cff736
Reporter | ||
Comment 15•13 years ago
|
||
(In reply to comment #13) > (In reply to comment #12) > > R- only because the introduction of the drc variable in policy manager persists > > (though it's never used that way). Feel free to assume R+ after that's been > > fixed. > > I think you are getting tripped up by the fact that GCConfig is defined in > GCPolicyManager.h. Right you are, I should have noticed the context.
Reporter | ||
Updated•13 years ago
|
Attachment #529522 -
Flags: review- → review+
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 16•13 years ago
|
||
tamarin-redux-serrano brbaker$ hg log -r 527e40959b44 changeset: 6239:527e40959b44 user: Steven Johnson <stejohns@adobe.com> date: Mon May 09 11:46:17 2011 -0700 summary: Bug 603606 - Disable DRC switch (r=lhansen) [manual transplant of 1e5dc1cff736 from tamarin-redux]
You need to log in
before you can comment on or make changes to this bug.
Description
•