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)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q3 11 - Serrano

People

(Reporter: lhansen, Assigned: treilly)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Disable DRC (obsolete) — Splinter Review
Passes acceptance testing in Debug and ReleaseDebugger builds of current TR tip.
I would have put the short circuit in Add but this works.
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
Attached patch Disable DRC, v2 (obsolete) — Splinter Review
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
Priority: P3 → --
Target Milestone: flash10.x - Serrano → Future
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Whiteboard: has-patch
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.
(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...
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
Whiteboard: has-patch
Assignee: nobody → treilly
Attachment #529343 - Flags: review?(lhansen)
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-
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)
Attachment #529510 - Attachment is obsolete: true
Attachment #529510 - Flags: review?(lhansen)
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)
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-
(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.
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
(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.
Attachment #529522 - Flags: review- → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: