Status

Tamarin
Garbage Collection (mmGC)
8 years ago
8 years ago

People

(Reporter: Tommy Reilly, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Future
x86
Mac OS X
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Finding DRC bugs can be made easier if we reap the ZCT after every add

Updated

8 years ago
Flags: flashplayer-qrb+
Target Milestone: --- → Future
(Reporter)

Comment 1

8 years ago
Created attachment 445104 [details] [diff] [review]
adds greedy DRC capabilities

for safe keeping I'm using this in my player testing I may push to Argo so QE can use it (hence the env var switch)
Attachment #445104 - Flags: review?(lhansen)
(Reporter)

Updated

8 years ago
Attachment #445104 - Flags: feedback?(fklockii)

Comment 2

8 years ago
Comment on attachment 445104 [details] [diff] [review]
adds greedy DRC capabilities

(General remark, applies cleanly to Argo but not at all to Redux, given space/tab issues.  I can deal with this.)

GC.h:831, kGreedyDRC almost certainly does not belong in that enum, a separate enum would be appropriate (since DRC mode composes with GC mode, at least some of the time).  I know, it complicates the GC API a little bit.

GC.cpp:962, strongly disklike the use of getenv here.  Must we?  Just the fact that we're reading environment variables at all in the GC makes me nervous, as we're not validating what we read very well.

GC.h:1475, Typo in comment, "so" should be "go". 

ZCT.h:225, want separate documentation for EnterSlowState and LeaveSlowState.

ZCT.cpp, this refactoring seems reasonable.

ZCT-inlines.h: I doubt there's any point in moving StartCollecting and EndCollecting into this file, since they're nowhere near performance critical and separating them from the rest of the logic around state management just makes the code harder to grok.  Suggest these go back into the cpp file.

Shell changes all look fine.

(Will leave review flag as-is for now and await some replies.)
I'm with lars on the getenv thing.  Tommy: do you anticipate QE using environment variables to turn this mode on?  (I am trying to interpret what you said in comment 1.)

Also I'd get rid of the two whitespace-only deltas in the patch (because I like concise patches).
(Reporter)

Comment 4

8 years ago
I'm finding more issues as a use it, I'll post another cleaner patch before I leave.   I really like using env vars although I know its somewhat unprecedented.   Having one way to control the GC whether its the player or the shell is the biggest attraction.   Second attraction is having a "per-instance" way to control the player since mms.cfg files are global.   I'm not in love with env vars but I'm in love with the convienence/flexibility I'm getting out of them ;-)
Maybe factor the getenv bit into a separate patch then, if its not targeted at QE but is rather meant for hacking by individuals.  (I can certainly see the attraction of using an env var to control this; I just want to guard against it accidentally ending up in the codebase.)

Comment 6

8 years ago
Comment on attachment 445104 [details] [diff] [review]
adds greedy DRC capabilities

Awaiting a new patch.
Attachment #445104 - Flags: review?(lhansen)
Attachment #445104 - Flags: feedback?(fklockii)

Updated

8 years ago
Blocks: 604350
You need to log in before you can comment on or make changes to this bug.