Closed Bug 608952 Opened 14 years ago Closed 14 years ago

Add shell switch to set GCPolicyManager::collectionThreshold

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
Q3 11 - Serrano

People

(Reporter: brbaker, Assigned: pnkfelix)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently there is no way to modify the collectionThreshold [1] which is hardcoded to a value if 512 blocks (1MB). This is fine for normal usage but in order to test the GC more using the acceptance testsuite I need to lower the threshold. With the default value, only 104 of the abc files in the testsuite of 2471 files cause a colllection cycle to happen. If the value of the threshold is changed to 256 blocks then 2430 of the abc files will cause a collection to happen.

[1] The collectionThreshold is the lower limit beyond which we try not to run the garbage collector.
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
(In reply to comment #0)
I miss quoted the default value of collectionThreshold, default number of blocks is 256, and the change I made to increase collections was to set the value to 128.
This should be a quick fix, would be good to have it sooner rather than later.

(Also would be good to expose it somehow in the Flash Player, obviously (if only in special hand-crafted builds?) but that may be a separate bug - we also want to expose the load factor settings to the Flash Player.)
Assignee: nobody → fklockii
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → flash10.x - Serrano
There's two basic ways I see of going about this.

1. One is to extend the GCPolicyManager constructor to the client to specify the threshold, and then have the shell invoke the constructor accordingly.  This is simple and should replicate Brent's experimental setup.

2. The other is to have the shell invoke GCPolicyManager::setLowerLimitCollectionThreshold; but this alone does not suffice, because the change in threshold will not be reflected in the runtime's behavior until after the next major collection occurs -- but the whole problem here is that a major collection is not happening in the first place!

2a. So you can modify this approach and change GCPolicyManager::setLowerLimitCollectionThreshold to actually adjust the budgets in response to the change in threshold.  I thought this would be easy, but it is looking like it is not, in part because the budget calculations are ever so slightly different between the initial policy calculation (GCPolicyManager::adjustPolicyInitially) and the ongoing policy calculation (GCPolicyManager::adjustPolicyForNextMajorCycle, GCPolicyManager::adjustPolicyForNextMinorCycle)

I'm going to spend a little bit more time on 2a, mostly as a learning exercise.  But Lars/Tommy: who are the clients of GCPolicyManager::setLowerLimitCollectionThreshold?  Would it break things to have that function adjusting the allocation budgets?
Bug 608952: Give shell ability to adjust collection threshold.

Part of this is making setLowerLimitCollectionThreshold itself
dynamically adjust the collection budgets, which may be a misguided
idea.
Attachment #488566 - Flags: feedback?(treilly)
I suggest you kill setLowerLimitCollectionThreshold since it's ill-defined and you extend the constructor chain to allow the lower limit to be specified at construction time either as an argument or part of a configuration structure.
Comment on attachment 488566 [details] [diff] [review]
Give shell ability to adjust collection threshold.

obsoleting questionable patch and canceling feedback request in response to comment 5.

will be taking the other tack shortly.
Attachment #488566 - Attachment is obsolete: true
Attachment #488566 - Flags: feedback?(treilly)
A bulk of this is the addition of a GCConfig class which should ease
similar future additions (such as the load factor parameterization
suggested by Lars).
Attachment #488888 - Flags: superreview?(lhansen)
Attachment #488888 - Flags: review?(treilly)
Attachment #488888 - Flags: feedback?(brbaker)
One question I'd like resolved: currently this makes the flag "-Dgcthreshold" rather than "-gcthreshold".  I'm still not clear on which flags are supposed to start with "-D" and which are not.

Also, I just realized that I should have included documentation for the flag with the patch in attachment 488888 [details] [diff] [review].  I'll whip some up.
Attachment #488890 - Flags: superreview?(lhansen)
Attachment #488890 - Flags: review?(treilly)
(In reply to comment #5)
> I suggest you kill setLowerLimitCollectionThreshold since it's ill-defined

Cannot immediately kill off setLowerLimitCollectionThreshold since it is invoked from the player side.

I assert it is outside the scope of this bug to remove the method invocations from the player; let me know if I am wrong.
(In reply to comment #10)
> (In reply to comment #5)
> > I suggest you kill setLowerLimitCollectionThreshold since it's ill-defined
> 
> Cannot immediately kill off setLowerLimitCollectionThreshold since it is
> invoked from the player side.
> 
> I assert it is outside the scope of this bug to remove the method invocations
> from the player; let me know if I am wrong.

I'll grant you that.  Please file a follow-up bug on that, since you've demonstrated earlier that the method is ill-defined in general (even though it probably works OK in the Player's case where the lower limit is raised, if I'm not mistaken).
Comment on attachment 488890 [details] [diff] [review]
doc for the new switch

It may be time to clean up switch names, since -Dgcthreshold (not an ideal name, ambiguous, actually misleading) pairs up in some way with -memlimit (not an ideal name, ambiguous, actually misleading).  And there is no "memory lower limit" (if that makes sense - it might) nor is there a way of limiting the memory for a given GC instance at the upper end except indirectly through -memlimit.  And there's the -D mess.

Thank god for good documentation!
Attachment #488890 - Flags: superreview?(lhansen) → superreview+
Comment on attachment 488888 [details] [diff] [review]
Give shell ability to adjust collection threshold.

It may be a slight simplification here to require a non-null config structure to be passed to GCPolicyManager, for GC::GC to create that structure if the pointer it receives is NULL (yes, maybe tricky, maybe not), and for the default policy values to be encoded all in the GCConfig's contructor, and for '0' not to have any special meaning.  However, that can be changed now or later without actually affecting any external APIs so I don't see it as a big deal.
Attachment #488888 - Flags: superreview?(lhansen) → superreview+
(In reply to comment #12)
> And there is no "memory lower limit" (if that makes sense - it might)

You're right; reusing the word "threshold" is (probably) more appropriate than saying "lower limit" here.  My brain was fogged from seeing LowerLimit in the GCPolicyManager methods.

In light of your other comment about potentially removing the special meaning of 0 here, do you want me to get rid of tying 0 to meaning "use default" in the switch (e.g. instead just say N must be positive)?
I think "threshold" is no more accurate than "lower limit"; in neither case is it entirely clear what's going on.  IIRC the mechanism we're controlling is the starting of an incremental GC; the incremental GC will not start so long as the heap size is below that setting.  (Once an incremental GC is going the setting is ignored, I believe?)  If that's the behavior it's controlling then that's probably what the docs should say.

If it were me I would not allow "0" to mean default; the default can be had by omitting the switch.
Attachment #488888 - Flags: feedback?(brbaker) → feedback+
Blocks: 610656
Attachment #488888 - Flags: review?(treilly)
Attachment #488890 - Flags: review?(treilly)
Please push this.
(In reply to comment #16)
> Please push this.

TR changeset - 5534:6310daf22ee8
http://hg.mozilla.org/tamarin-redux/rev/6310daf22ee8

(hope the doc for switch is better now!)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: flashplayer-qrb?
Status: RESOLVED → VERIFIED
Flags: flashplayer-bug-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: