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

Create a switch to disable DRC by doing nothing during reaping

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Garbage Collection (mmGC)
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Lars T Hansen, Assigned: Tommy Reilly)

Tracking

(Blocks: 1 bug)

unspecified
Q3 11 - Serrano

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Created attachment 482559 [details] [diff] [review]
Disable DRC

Passes acceptance testing in Debug and ReleaseDebugger builds of current TR tip.
(Assignee)

Comment 2

7 years ago
I would have put the short circuit in Add but this works.
(Reporter)

Comment 3

7 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

7 years ago
Created attachment 483152 [details] [diff] [review]
Disable DRC, v2

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

7 years ago
Priority: P3 → --
Target Milestone: flash10.x - Serrano → Future
(Reporter)

Updated

7 years ago
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Whiteboard: has-patch
(Assignee)

Comment 5

7 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

7 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

7 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

7 years ago
Whiteboard: has-patch
(Reporter)

Updated

7 years ago
Assignee: nobody → treilly
(Assignee)

Comment 8

7 years ago
Created attachment 529343 [details] [diff] [review]
MMGC_DRC env var to disable drc set to 0
Attachment #529343 - Flags: review?(lhansen)
(Reporter)

Comment 9

7 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

7 years ago
Created attachment 529510 [details] [diff] [review]
Take 2, cleaned up and also linked up sensibly to nogc flag

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

7 years ago
Attachment #529510 - Attachment is obsolete: true
Attachment #529510 - Flags: review?(lhansen)
(Assignee)

Comment 11

7 years ago
Created attachment 529522 [details] [diff] [review]
Take 2, cleaned up and also linked up sensibly to nogc mode

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

7 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

7 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

7 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

7 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

7 years ago
Attachment #529522 - Flags: review- → review+
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 16

6 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.