Moodle
  1. Moodle
  2. MDL-32379

Memory leak in session_set_user (forum digest hits memory limit)

    Details

    • Testing Instructions:
      Hide

      Not sure how to test this. Adding test instructions to make sure forum cron doesn't break.

      1. Enroll 5 users to a course.
      2. Add forum to course and set Subscription mode to Forced subscription
      3. Post discussion to forum by logging as different user
      4. Run cron and make sure every one gets email.
      Show
      Not sure how to test this. Adding test instructions to make sure forum cron doesn't break. Enroll 5 users to a course. Add forum to course and set Subscription mode to Forced subscription Post discussion to forum by logging as different user Run cron and make sure every one gets email.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w21_MDL-32379_m23_forumcron
    • Rank:
      39235

      Description

      The session_set_user function points $_SESSION['USER'] at the passed object.

      When data is attached to the session user, this bloats out the referenced object, so if a process changes users many times from say, a prefilled list of records, it will hit the memory limit and fail. For example, the forum digest sending is affected by this problem and will fail on any site where a large number of users are to be sent a digest.

      This can be resolved by simply cloning the passed $user object to break the reference.

      Github incoming with a patch.

      1. sc.php
        0.5 kB
        Ankit Agarwal

        Issue Links

          Activity

          Show
          Tony Levi added a comment - https://github.com/tlevi/moodle/compare/mdl32379_23
          Hide
          Tony Levi added a comment -

          updating and raising to critical; breaks forum digests on any large site.

          Show
          Tony Levi added a comment - updating and raising to critical; breaks forum digests on any large site.
          Hide
          Michael de Raadt added a comment -

          Thanks for discovering that and providing a fix.

          Show
          Michael de Raadt added a comment - Thanks for discovering that and providing a fix.
          Hide
          Rajesh Taneja added a comment -

          Adding this to next sprint

          Show
          Rajesh Taneja added a comment - Adding this to next sprint
          Hide
          Rajesh Taneja added a comment -

          Thanks for spot-on patch, pushing for peer-review

          Show
          Rajesh Taneja added a comment - Thanks for spot-on patch, pushing for peer-review
          Hide
          Ankit Agarwal added a comment -

          Hi,
          Patch looks good.
          I wrote a simple script as proof of concept for this issue.
          You can run the script attached with both with and without the patch. Without patch you can actually see the memory limit growing with each call to the function, where it is constant with the patch applied.

          The reason for the increase is mainly because $_SESSION['user'] is a reference to original $user so all data that is added to $_SESSION['user'] is done to the original array. which inflates the original array.

          +1 for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi, Patch looks good. I wrote a simple script as proof of concept for this issue. You can run the script attached with both with and without the patch. Without patch you can actually see the memory limit growing with each call to the function, where it is constant with the patch applied. The reason for the increase is mainly because $_SESSION ['user'] is a reference to original $user so all data that is added to $_SESSION ['user'] is done to the original array. which inflates the original array. +1 for integration. Thanks
          Hide
          Ankit Agarwal added a comment -

          attaching the test script.
          Edit the path and run from cli
          Thanks

          Show
          Ankit Agarwal added a comment - attaching the test script. Edit the path and run from cli Thanks
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for the Review and Script, Ankit

          Show
          Rajesh Taneja added a comment - - edited Thanks for the Review and Script, Ankit
          Hide
          Petr Škoda added a comment -

          Hi, this behaviour was intentional and the change will probably degrade performance on sites where the data fits into memory. The stuff is $USER is cached data, not bloat.

          Show
          Petr Škoda added a comment - Hi, this behaviour was intentional and the change will probably degrade performance on sites where the data fits into memory. The stuff is $USER is cached data, not bloat.
          Hide
          Tony Levi added a comment -

          It isn't useful at all where the user is touched once and it is hardly expected or predictable behavior that an object would be jammed with magic caches by the callee.

          Is there a real case where performance is degraded?

          Show
          Tony Levi added a comment - It isn't useful at all where the user is touched once and it is hardly expected or predictable behavior that an object would be jammed with magic caches by the callee. Is there a real case where performance is degraded?
          Hide
          Petr Škoda added a comment - - edited

          I can not talk about 2.x cron because I worked on older versions only, previously it was helping because it was not very efficient, maybe it was improved to not loop the users array multiple times in all modules.

          In any case please do not touch session_set_user() and instead tweak cron_setup_user() only if necessary, the reason is there is no way to verify how it is used in existing code including contrib and this change might theoretically cause some very nasty regressions in session related code.

          Show
          Petr Škoda added a comment - - edited I can not talk about 2.x cron because I worked on older versions only, previously it was helping because it was not very efficient, maybe it was improved to not loop the users array multiple times in all modules. In any case please do not touch session_set_user() and instead tweak cron_setup_user() only if necessary, the reason is there is no way to verify how it is used in existing code including contrib and this change might theoretically cause some very nasty regressions in session related code.
          Hide
          Petr Škoda added a comment - - edited

          Hmm, I am still not persuaded we should blame session_set_user() or cron_setup_user() because they work consistently with the rest of session and accesslib API - we always add caches to $user record when submitted as function parameter. The forum cron should be imo blamed because it should not try to create the huge array of user records in the first place. This "over-optimisation" is most probably my fault, at that time it helped moodle.org, but I agree that is a sloppy coding style. Ideally we should store only user->id in array and fetch the user records from some fast cache, PHP with its crazy array implementation does not help much here. Possible workaround might be to just clone the user record right before calling cron_setup_user(), it the long term we should imho eliminate the huge user array completely.

          In any case thanks a lot for the report, patch and especially your cooperation! I agree this needs to be somehow fixed in all supported stable branches asap.

          Show
          Petr Škoda added a comment - - edited Hmm, I am still not persuaded we should blame session_set_user() or cron_setup_user() because they work consistently with the rest of session and accesslib API - we always add caches to $user record when submitted as function parameter. The forum cron should be imo blamed because it should not try to create the huge array of user records in the first place. This "over-optimisation" is most probably my fault, at that time it helped moodle.org, but I agree that is a sloppy coding style. Ideally we should store only user->id in array and fetch the user records from some fast cache, PHP with its crazy array implementation does not help much here. Possible workaround might be to just clone the user record right before calling cron_setup_user(), it the long term we should imho eliminate the huge user array completely. In any case thanks a lot for the report, patch and especially your cooperation! I agree this needs to be somehow fixed in all supported stable branches asap.
          Hide
          Tony Levi added a comment -

          Hmmm yes but forum cron may not be the only one afflicted with this problem; it would be nice to solve all in one place. If session_set_user or cron setup user is not changed, there should at least be some clear documentation that the object will be modified with heaps of extra data.

          This is living in our stables for a while now so if there is any extra slowness or regression somewhere we'll know; so far nothing.

          Aside - forum cron itself needs a revisit eventually, it is causing some pain too.

          Show
          Tony Levi added a comment - Hmmm yes but forum cron may not be the only one afflicted with this problem; it would be nice to solve all in one place. If session_set_user or cron setup user is not changed, there should at least be some clear documentation that the object will be modified with heaps of extra data. This is living in our stables for a while now so if there is any extra slowness or regression somewhere we'll know; so far nothing. Aside - forum cron itself needs a revisit eventually, it is causing some pain too.
          Hide
          Dan Poltawski added a comment -

          Hi everyone,

          Thanks for the work - this has been integrated now.

          Perhaps we should create a new issue to investigate the root cause.

          Show
          Dan Poltawski added a comment - Hi everyone, Thanks for the work - this has been integrated now. Perhaps we should create a new issue to investigate the root cause.
          Hide
          Petr Škoda added a comment -

          Dan: did you read all my comments? Please revert it or at least move the cloning to cron_setup_user().

          Show
          Petr Škoda added a comment - Dan: did you read all my comments? Please revert it or at least move the cloning to cron_setup_user().
          Hide
          Dan Poltawski added a comment -

          Grr, my apologies.

          I did read your comments and I thought that the change had been made. But clearly didn't read the diff properly.

          Sorry everyone, reverting.

          Show
          Dan Poltawski added a comment - Grr, my apologies. I did read your comments and I thought that the change had been made. But clearly didn't read the diff properly. Sorry everyone, reverting.
          Hide
          Dan Poltawski added a comment -

          Sorry for the noise. I've reverted this change - I shouldn't have integrated this.

          Reopening.

          Show
          Dan Poltawski added a comment - Sorry for the noise. I've reverted this change - I shouldn't have integrated this. Reopening.
          Hide
          Petr Škoda added a comment -

          No worries dan, at least we have one more person deeply interested in this issue. Which of the options do you like most:
          1/ clone in session_set_user() - I am personally worried about this one
          2/ clone in cron_setup_user() - it should resolve this forum problem, some optimisations where $user is used repeatedly will stop work (that is both good and bad depending on the optimisations)
          3/ clone $user before cron_setup_user() in forum cron - this fixes only forum code
          4/ we could add new parameter to cron_setup_user() for $cloneuser=true - forum would be resolved for now, other places that really want to use the caching could enable it

          Show
          Petr Škoda added a comment - No worries dan, at least we have one more person deeply interested in this issue. Which of the options do you like most: 1/ clone in session_set_user() - I am personally worried about this one 2/ clone in cron_setup_user() - it should resolve this forum problem, some optimisations where $user is used repeatedly will stop work (that is both good and bad depending on the optimisations) 3/ clone $user before cron_setup_user() in forum cron - this fixes only forum code 4/ we could add new parameter to cron_setup_user() for $cloneuser=true - forum would be resolved for now, other places that really want to use the caching could enable it
          Hide
          Rajesh Taneja added a comment -

          +1 for point 4 (Option to clone user and set it to true for forum (for now)).

          Show
          Rajesh Taneja added a comment - +1 for point 4 (Option to clone user and set it to true for forum (for now)).
          Hide
          Dan Poltawski added a comment -

          I prefer 2) but I don't know example of where we are optimising to take advantage of this.

          Show
          Dan Poltawski added a comment - I prefer 2) but I don't know example of where we are optimising to take advantage of this.
          Hide
          Petr Škoda added a comment -

          Where do we depend on this? see forum cron:

          foreach ($users as $userto) {
              cron_setup_user($userto);
              ...
              foreach ($posts as $pid => $post) {
                  cron_setup_user($userto, $course);
                  ....
              }
          }
          

          the inner loop benefits from the caching because it does not have do the very expensive USER->access caching repeatedly for the same user.

          Show
          Petr Škoda added a comment - Where do we depend on this? see forum cron: foreach ($users as $userto) { cron_setup_user($userto); ... foreach ($posts as $pid => $post) { cron_setup_user($userto, $course); .... } } the inner loop benefits from the caching because it does not have do the very expensive USER->access caching repeatedly for the same user.
          Hide
          Petr Škoda added a comment - - edited

          Hmm, we might be actually caching the access data in another cache, but still user preferences necessary for messaging would be refetched. I am wondering if the clone is actually going to solve much because the accesslib caching would not be prevented by that much (see $ACCESSLIB_PRIVATE->accessdatabyuser[$userid]).

          Show
          Petr Škoda added a comment - - edited Hmm, we might be actually caching the access data in another cache, but still user preferences necessary for messaging would be refetched. I am wondering if the clone is actually going to solve much because the accesslib caching would not be prevented by that much (see $ACCESSLIB_PRIVATE->accessdatabyuser [$userid] ).
          Hide
          Petr Škoda added a comment -

          Rehmmm, maybe we could dedupe the role definitions in CLI scripts to reduce memory usage, it might save some memory, but it would consume CPU cycles.

          Show
          Petr Škoda added a comment - Rehmmm, maybe we could dedupe the role definitions in CLI scripts to reduce memory usage, it might save some memory, but it would consume CPU cycles.
          Hide
          Petr Škoda added a comment -

          I wish I had some real data for testing here...

          Show
          Petr Škoda added a comment - I wish I had some real data for testing here...
          Hide
          Rajesh Taneja added a comment -

          Looking at this again, can we optimise this a bit more by removing:

          1. Assignments and using actual variables.
            $userto = $users[$userid];
          2. After setting the session variable, unset $userto and use $_SESSION['USER']

          This will avoid any extra copies and will optimise memory.

          What do you think Petr?

          Show
          Rajesh Taneja added a comment - Looking at this again, can we optimise this a bit more by removing: Assignments and using actual variables. $userto = $users [$userid] ; After setting the session variable, unset $userto and use $_SESSION ['USER'] This will avoid any extra copies and will optimise memory. What do you think Petr?
          Hide
          Petr Škoda added a comment -

          Rajesh: I do not understand what you are proposing.

          Show
          Petr Škoda added a comment - Rajesh: I do not understand what you are proposing.
          Hide
          Rajesh Taneja added a comment -

          Sorry Petr,
          My Bad... was thinking in wrong direction.

          I will go with 3) for now, and backport this. Hope that will solve this issue and acceptable by everyone.

          Show
          Rajesh Taneja added a comment - Sorry Petr, My Bad... was thinking in wrong direction. I will go with 3) for now, and backport this. Hope that will solve this issue and acceptable by everyone.
          Hide
          Ankit Agarwal added a comment -

          Hi Rajesh,
          ->The patch looks good, but as Petr pointed out this will degrade performance in sites where all data fits into memory. But I don't think we have any alternative.
          ->User data is still cached in $ACCESSLIB_PRIVATE when the cap checks are made. Not sure if that is going to create problems or not, but for sure this needs to be tested with some large data before we can assume $ACCESSLIB_PRIVATE is not going to create any problem.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Rajesh, ->The patch looks good, but as Petr pointed out this will degrade performance in sites where all data fits into memory. But I don't think we have any alternative. ->User data is still cached in $ACCESSLIB_PRIVATE when the cap checks are made. Not sure if that is going to create problems or not, but for sure this needs to be tested with some large data before we can assume $ACCESSLIB_PRIVATE is not going to create any problem. Thanks
          Hide
          Tony Levi added a comment -

          Petr: what would 'real data' be? Need a DB from a large site with these problems?

          Show
          Tony Levi added a comment - Petr: what would 'real data' be? Need a DB from a large site with these problems?
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,

          Petr also mentioned about this, but I am not sure how to get around this. I think this can be a bottleneck, but not sure as I can't test this.

          @Tony:
          Can you please test the patch and see if this helps.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, Petr also mentioned about this, but I am not sure how to get around this. I think this can be a bottleneck, but not sure as I can't test this. @Tony: Can you please test the patch and see if this helps.
          Hide
          Tony Levi added a comment -

          Sorry, don't have the test data and setup that I needed to track this anymore, and it is resolved very well for us so I have to move on.

          Show
          Tony Levi added a comment - Sorry, don't have the test data and setup that I needed to track this anymore, and it is resolved very well for us so I have to move on.
          Hide
          Tony Levi added a comment -

          though, FWIW, I think Petr's concerns are theoretical at best.

          Show
          Tony Levi added a comment - though, FWIW, I think Petr's concerns are theoretical at best.
          Hide
          Petr Škoda added a comment - - edited

          The patch is not ok, my point was to NOT clone in that inner loops.

          Show
          Petr Škoda added a comment - - edited The patch is not ok, my point was to NOT clone in that inner loops.
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr,

          So it should be done like this?

          foreach ($users as $userto) {
              cron_setup_user(clone $userto);
              ...
              foreach ($posts as $pid => $post) {
                  cron_setup_user($userto, $course);
                  ....
              }
          }
          

          and

          foreach ($users as $userto) {
              cron_setup_user(clone $userto);
              ...
              foreach ($thesediscussions as $discussionid) {
                  cron_setup_user($userto, $course);
                  ....
              }
          }
          
          Show
          Rajesh Taneja added a comment - Thanks Petr, So it should be done like this? foreach ($users as $userto) { cron_setup_user(clone $userto); ... foreach ($posts as $pid => $post) { cron_setup_user($userto, $course); .... } } and foreach ($users as $userto) { cron_setup_user(clone $userto); ... foreach ($thesediscussions as $discussionid) { cron_setup_user($userto, $course); .... } }
          Hide
          Rajesh Taneja added a comment -

          I am not sure, if cloning $userto for outer loop will help.
          At this point, I can't find any duplicate as well for this issue. So rather then using half-way solution (which I am not sure at this point), I would propose to look at this later when MUC is done.

          In the mean while, I will close this as "Will not fix", and open a sub-task for MDL-23754.

          Show
          Rajesh Taneja added a comment - I am not sure, if cloning $userto for outer loop will help. At this point, I can't find any duplicate as well for this issue. So rather then using half-way solution (which I am not sure at this point), I would propose to look at this later when MUC is done. In the mean while, I will close this as "Will not fix", and open a sub-task for MDL-23754 .
          Hide
          Rajesh Taneja added a comment -

          Hello Tony and Petr,

          Is it fine with you, if I close this issue and open sub-task for MDL-23754. I am not sure about this half-way solution.

          Show
          Rajesh Taneja added a comment - Hello Tony and Petr, Is it fine with you, if I close this issue and open sub-task for MDL-23754 . I am not sure about this half-way solution.
          Hide
          Mark Drechsler added a comment -

          Rajesh,

          Appreciate Petr's passion to find the most elegant solution possible, but this is a significant issue for many of our large clients right now, and the fix Tony has provided fixes that issue right now on all our large sites, with no identifiable side effects that we have seen after running this in real, live, large sites.

          We'll continue to run this in our managed environments, but to me this would seem to be a good interim fix for a proven performance issue in core while a more 'perfect' solution is debated.

          Thoughts?

          Mark.

          Show
          Mark Drechsler added a comment - Rajesh, Appreciate Petr's passion to find the most elegant solution possible, but this is a significant issue for many of our large clients right now , and the fix Tony has provided fixes that issue right now on all our large sites, with no identifiable side effects that we have seen after running this in real, live, large sites. We'll continue to run this in our managed environments, but to me this would seem to be a good interim fix for a proven performance issue in core while a more 'perfect' solution is debated. Thoughts? Mark.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the quick reply Mark,

          Unfortunately, we don't have a large site, on which we can test this solution.
          Is it possible for you to test attached patch and let us know if it helps.

          Show
          Rajesh Taneja added a comment - Thanks for the quick reply Mark, Unfortunately, we don't have a large site, on which we can test this solution. Is it possible for you to test attached patch and let us know if it helps.
          Hide
          Adam Olley added a comment -

          >In the mean while, I will close this as "Will not fix"

          "Will not fix" a bug that consistently causes cron to throw OOM errors and sends out duplicate mail on every single cron run until the error is caught? I get that MUC might be able to handle it, but MUC is still a far ways off and a possible solution in the future is no reason to ignore critical bugs now (and really, given that with enough users to send mail to, it will fatal error, every time, this should probably be a blocker).

          Show
          Adam Olley added a comment - >In the mean while, I will close this as "Will not fix" "Will not fix" a bug that consistently causes cron to throw OOM errors and sends out duplicate mail on every single cron run until the error is caught? I get that MUC might be able to handle it, but MUC is still a far ways off and a possible solution in the future is no reason to ignore critical bugs now (and really, given that with enough users to send mail to, it will fatal error, every time, this should probably be a blocker).
          Hide
          Dan Poltawski added a comment -

          Hi Netspot guys - I agree we shouldn't close this - lets try and work together for a solution

          Show
          Dan Poltawski added a comment - Hi Netspot guys - I agree we shouldn't close this - lets try and work together for a solution
          Hide
          Petr Škoda added a comment -

          Let me sum it up: the original proposed "fix" would create performance regression in the inner loop in forum cron and it would not resolve the caching problems in accesslib and it would not completely prevent memory exhaustion in forum cron caused by the user array iteration and it might create regressions elsewhere. The added "clone" in outer loop should conserve the same amount of memory in forum cron as the original proposed patch.

          My +10 to improve cron performance and lower memory consumption, I personally do not have access to any real large server or backup data from it. I wrote the original code and I agree it is far from optimal, workaround is to allow more memory in cron, that was one of the reasons why we introduced $CFG->extramemorylimit - large installs can not run with default settings, sorry.

          Show
          Petr Škoda added a comment - Let me sum it up: the original proposed "fix" would create performance regression in the inner loop in forum cron and it would not resolve the caching problems in accesslib and it would not completely prevent memory exhaustion in forum cron caused by the user array iteration and it might create regressions elsewhere. The added "clone" in outer loop should conserve the same amount of memory in forum cron as the original proposed patch. My +10 to improve cron performance and lower memory consumption, I personally do not have access to any real large server or backup data from it. I wrote the original code and I agree it is far from optimal, workaround is to allow more memory in cron, that was one of the reasons why we introduced $CFG->extramemorylimit - large installs can not run with default settings, sorry.
          Hide
          Tony Levi added a comment -

          There is no noticeable speed difference in our production environments.

          What you are talking about is a theoretical micro-optimisation and has no application.

          Unbounded memory use for no real reason is a bug, regardless of magical configuration options.

          Show
          Tony Levi added a comment - There is no noticeable speed difference in our production environments. What you are talking about is a theoretical micro-optimisation and has no application. Unbounded memory use for no real reason is a bug, regardless of magical configuration options.
          Hide
          Dan Poltawski added a comment -

          Just a thought around this that perhaps the forum loop would benefit from a gc_collect_cycles() to help clear up the memory.

          The OU guys found this was necessary in MDL-32214

          Show
          Dan Poltawski added a comment - Just a thought around this that perhaps the forum loop would benefit from a gc_collect_cycles() to help clear up the memory. The OU guys found this was necessary in MDL-32214
          Hide
          Tony Levi added a comment -

          No, the memory is attached to real live objects; this won't help.

          Show
          Tony Levi added a comment - No, the memory is attached to real live objects; this won't help.
          Hide
          Petr Škoda added a comment -

          Tony: I did not say it is "not a bug", I was only explaining why the first proposed patch was problematic. It would be really great if somebody improved the forum cron or even completely redesigned the cron execution. Please note this is GPL code where you have no guarantees, if there are bugs you can not demand them to be fixed, but you can do whatever you want with your copy of the code (well, things that GNU GPL 3 allows you). If somebody comes with better fix I guess there are not going to be any problems integrating it into moodle distribution that is available from moodle.org and if the risks are small it may be applied to all stable branches.

          Show
          Petr Škoda added a comment - Tony: I did not say it is "not a bug", I was only explaining why the first proposed patch was problematic. It would be really great if somebody improved the forum cron or even completely redesigned the cron execution. Please note this is GPL code where you have no guarantees, if there are bugs you can not demand them to be fixed, but you can do whatever you want with your copy of the code (well, things that GNU GPL 3 allows you). If somebody comes with better fix I guess there are not going to be any problems integrating it into moodle distribution that is available from moodle.org and if the risks are small it may be applied to all stable branches.
          Hide
          Tony Levi added a comment - - edited

          Oh we have forked already, I just figured upstream wanted some contributions.

          I see that throughly isn't the case, based purely on theoretical proven false objections yet again, so I'll leave you to it.

          P.S. This here is how all good software projects die.

          Show
          Tony Levi added a comment - - edited Oh we have forked already, I just figured upstream wanted some contributions. I see that throughly isn't the case, based purely on theoretical proven false objections yet again, so I'll leave you to it. P.S. This here is how all good software projects die.
          Hide
          Petr Škoda added a comment - - edited

          oh, and "What you are talking about is a theoretical micro-optimisation and has no application." was not true in case of moodle.org when the code was introduced (around 1.8), it may not be the case now, but to me it looks like the current USER->access init is more expensive than before, but again I have no idea how much the messaging costs now, how does the rendering work, etc. In any case integrators and testers need to have a way to prove the optimisation works, the attached sc.php file is imo not very relevant to cron because it does not use capabilities and messaging, it just illustrates PHP object reference behaviour. We should collect some hard data with memory use, number of db queries+their type, execution times on different site types...

          Show
          Petr Škoda added a comment - - edited oh, and "What you are talking about is a theoretical micro-optimisation and has no application." was not true in case of moodle.org when the code was introduced (around 1.8), it may not be the case now, but to me it looks like the current USER->access init is more expensive than before, but again I have no idea how much the messaging costs now, how does the rendering work, etc. In any case integrators and testers need to have a way to prove the optimisation works, the attached sc.php file is imo not very relevant to cron because it does not use capabilities and messaging, it just illustrates PHP object reference behaviour. We should collect some hard data with memory use, number of db queries+their type, execution times on different site types...
          Hide
          Petr Škoda added a comment -

          Tony: I would like to thank you for all your contributions, it is really valuable and being appreciated. I am going to look at your other unresolved issues and try to help to get them through integration if possible.

          I have reviewed the patch by Rajesh now and I agree it is not correct solution either, I am going to work on my own patch now, I would really appreciate if you helped me with testing it later.

          Show
          Petr Škoda added a comment - Tony: I would like to thank you for all your contributions, it is really valuable and being appreciated. I am going to look at your other unresolved issues and try to help to get them through integration if possible. I have reviewed the patch by Rajesh now and I agree it is not correct solution either, I am going to work on my own patch now, I would really appreciate if you helped me with testing it later.
          Hide
          Petr Škoda added a comment - - edited

          here is the patch (untested), but it should show my direction: https://github.com/skodak/moodle/compare/w20_MDL-32379_m23_forumcron

          new features:

          • user cache size in cron is controlled by new constant that can be overridden from config.php
          • the user records are cloned before setting up session user in the outer foreach loop
          • smaller user record size by unsetting more properties that are not necessary
          Show
          Petr Škoda added a comment - - edited here is the patch (untested), but it should show my direction: https://github.com/skodak/moodle/compare/w20_MDL-32379_m23_forumcron new features: user cache size in cron is controlled by new constant that can be overridden from config.php the user records are cloned before setting up session user in the outer foreach loop smaller user record size by unsetting more properties that are not necessary
          Hide
          Petr Škoda added a comment -

          Submitted for integration, please test. I did not feel confident enough to request stable branches, but the code should cherry pick without problems if necessary. I hope this could solve it for now...

          Show
          Petr Škoda added a comment - Submitted for integration, please test. I did not feel confident enough to request stable branches, but the code should cherry pick without problems if necessary. I hope this could solve it for now...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Wow, this had somehow escaped my integration radar. Sorry.

          Integrated, thanks

          Show
          Dan Poltawski added a comment - Wow, this had somehow escaped my integration radar. Sorry. Integrated, thanks
          Hide
          Dan Poltawski added a comment -

          5 users were sent post 19

          Show
          Dan Poltawski added a comment - 5 users were sent post 19
          Hide
          Dan Poltawski added a comment -

          Looks good in my testing, but indeed this sort of thing is quite difficult to test, hence master only.

          Thanks everyone!

          Show
          Dan Poltawski added a comment - Looks good in my testing, but indeed this sort of thing is quite difficult to test, hence master only. Thanks everyone!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!
          Hide
          Dan Poltawski added a comment -

          There is a problem with moodle.org digests (MDLSITE-1834), still trying to diagnose this, but wonder if it could be caused by this issue.

          Show
          Dan Poltawski added a comment - There is a problem with moodle.org digests ( MDLSITE-1834 ), still trying to diagnose this, but wonder if it could be caused by this issue.
          Hide
          Dan Poltawski added a comment -

          Hi, just to note that i've setup a test RIG with 600,000 users and memory still doesn't seem to be scaling properly.

          I am investigating in MDL-23687

          Show
          Dan Poltawski added a comment - Hi, just to note that i've setup a test RIG with 600,000 users and memory still doesn't seem to be scaling properly. I am investigating in MDL-23687

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: