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)
Tamarin Graveyard
Garbage Collection (mmGC)
Tracking
(Not tracked)
VERIFIED
FIXED
Q3 11 - Serrano
People
(Reporter: brbaker, Assigned: pnkfelix)
References
Details
Attachments
(2 files, 1 obsolete file)
11.47 KB,
patch
|
lhansen
:
superreview+
brbaker
:
feedback+
|
Details | Diff | Splinter Review |
1.72 KB,
patch
|
lhansen
:
superreview+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•14 years ago
|
||
(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.
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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?
Assignee | ||
Comment 4•14 years ago
|
||
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)
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
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)
Assignee | ||
Comment 7•14 years ago
|
||
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)
Assignee | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #488890 -
Flags: superreview?(lhansen)
Attachment #488890 -
Flags: review?(treilly)
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Comment 11•14 years ago
|
||
(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 12•14 years ago
|
||
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 13•14 years ago
|
||
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+
Assignee | ||
Comment 14•14 years ago
|
||
(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)?
Comment 15•14 years ago
|
||
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.
Reporter | ||
Updated•14 years ago
|
Attachment #488888 -
Flags: feedback?(brbaker) → feedback+
Updated•14 years ago
|
Attachment #488888 -
Flags: review?(treilly)
Updated•14 years ago
|
Attachment #488890 -
Flags: review?(treilly)
Comment 16•14 years ago
|
||
Please push this.
Assignee | ||
Comment 17•14 years ago
|
||
(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!)
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•14 years ago
|
Flags: flashplayer-qrb?
Reporter | ||
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
Flags: flashplayer-bug-
You need to log in
before you can comment on or make changes to this bug.
Description
•