CDbHttpSession: garbage collection probability cannot be set low enough #486

Closed
tom-- opened this Issue Mar 11, 2012 · 12 comments

3 participants

Contributor
tom-- commented Mar 11, 2012

CDbHttpSession's configuration allows a minimum GC probability of 1%.

if a server is handling, e.g., 1000 requests a second, that means it runs GC 10 times a second. that's wasteful. in many cases once a minute would be sufficient so this minimum constraint could be off by two or three orders of magnitude.

suggest allowing the configuration of PHP's GC denominator.

@resurtm resurtm added a commit to resurtm/yii that referenced this issue Aug 25, 2012
@resurtm resurtm Enh #486: CHttpSession::$gCProbability and CDbHttpSession::$gCProbabi…
…lity are floats now. Minimal $gCProbability value has been changed to the 0.001%, was 1% before, default value left unchanged (1%).
e1af079
Contributor
resurtm commented Aug 25, 2012

@tom-- Could you please help with process of testing patch provided in PR #1288?

Contributor
tom-- commented Aug 25, 2012

Hi,

Thanks for doing this. Using a float is a great idea. Should be tolerable backwards compat. I will be glad to try to test but I have a couple of suggestions first.

First, bumping the divisor up to 100000 seems a little arbitrary to me. If Yii offers the user a float for $gCProbability, why not allow as much range as possible? So I would suggest maybe using 2147483647 as divisor (or even PHP_INT_MAX, I'm not sure).

Also, since there's no reason to believe that this VA is set before it is gotten (or that something else manipulates these php config vars) I think it would be safer for the getter to return something along the lines of:

ini_get('session.gc_probability')/ini_get('session.gc_divisor');

Let me know what you think.

Contributor
resurtm commented Aug 25, 2012

First, bumping the divisor up to 100000 seems a little arbitrary to me. If Yii offers the user a float for $gCProbability, why not allow as much range as possible? So I would suggest maybe using 2147483647 as divisor (or even PHP_INT_MAX, I'm not sure).

I think the value 2147483647 would be enough for any case and any situation user can have. And fixed value (2.1*10^9) would be better than PHP_INT_MAX because the latter is different across various platforms and architectures.

Also, since there's no reason to believe that this VA is set before it is gotten (or that something else manipulates these php config vars) I think it would be safer for the getter to return something along the lines of:

That makes sense. Thanks! Will fix it soon.

Contributor
resurtm commented Aug 25, 2012

@tom-- I'm done.

@cebe cebe was assigned Aug 29, 2012
@cebe cebe closed this Aug 29, 2012
Contributor
resurtm commented Aug 29, 2012

@tom-- So, fix was merged into upstream. Did you tried it?

Contributor
tom-- commented Aug 29, 2012

@resurtm not yet. the testing is going to require some sophistication and to estimate the probability will need statistical sampling and many runs. plus i should figure out first how to write a unit test that i can create a PR for. this week, i hope.

Contributor
tom-- commented Aug 29, 2012

as for the PR for the unit test, it's probably no different than any other, yes?
https://github.com/yiisoft/yii/wiki/Git-workflow-for-Yii-contributors

Contributor
resurtm commented Aug 29, 2012

@tom-- Okay. I'm also testing this issue.

Contributor
resurtm commented Aug 29, 2012

@tom-- Yes, the process completely same.

Contributor
tom-- commented Aug 29, 2012

should i include in one PR the new CTestCase script file and fixes to CHttpSession.php so that it passes the new tests?

Contributor
resurtm commented Aug 29, 2012

@tom-- yes, sure. That's ok.

@tom-- tom-- referenced this issue Aug 29, 2012
Merged

486 unit test #1309

Contributor
tom-- commented Aug 29, 2012

i added github code comments to the PRs diff view: #1309

@cebe cebe added a commit to cebe/yii that referenced this issue Sep 3, 2012
@cebe cebe Merge branch 'master' into 93-relational-beforefind-criteria-modifica…
…tion

* master: (180 commits)
  Updated changelog with last-modified date change.
  Changed the CHttpCacheFilter to use RFC 1123 complaint dates when returning the last-modified header.
  tabs...
  gii: better default validation "length" rule for fixed-point / floating-point field type (MYSQL)
  fixes #1319
  updated comment in CHttpSession
  Update framework/YiiBase.php
  Update docs/guide/database.arr.txt
  New unit test for enhanced $gCProbability s/getters in CHttpSession #486. Fixed those methods to process corner cases properly. Initial $gCProbability is now double(1.0), was > 1.0 before.
  Bug #112: MSSQL: database abstraction layer now uses native transaction support of the SQLSRV driver.
  Forgot about spaces in comments creating procedure call. [ci skip]
  Better comments testing method name.
  Forgot about local MSSQL database settings.
  MSSQL unit testing SQL file reformatted and decorated. MSSQL testing class improved. Added unit tests for column comments retrieving.
  Removed old message.
  Update docs/guide/topics.i18n.txt
  Update docs/guide/topics.i18n.txt
  Requirements checker: russian (ru_RU) messages.
  Minor fixes and refinements to the MSSQL unit tests.
  better fix for issues #124
  ...

Conflicts:
	UPGRADE
be1fbf1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment