565049 - Need a DRC greedy mode

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Tommy Reilly]

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

Comment 1

[Tommy Reilly]

Attachment: 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)

Comment 2

[Lars T Hansen]

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

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

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

Comment 4

[Tommy Reilly]

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 ;-)

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

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

[Lars T Hansen]

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

Awaiting a new patch.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.