Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

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

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

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

              Thanks for discovering that and providing a fix.

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

              Adding this to next sprint

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

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

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for spot-on patch, pushing for peer-review
              Hide
              ankit_frenz 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_frenz 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_frenz Ankit Agarwal added a comment -

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

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

              Thanks for the Review and Script, Ankit

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited Thanks for the Review and Script, Ankit
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              tlevi 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
              tlevi 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              tlevi 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
              tlevi 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
              poltawski 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
              poltawski 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - Dan: did you read all my comments? Please revert it or at least move the cloning to cron_setup_user().
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

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

              Reopening.

              Show
              poltawski Dan Poltawski added a comment - Sorry for the noise. I've reverted this change - I shouldn't have integrated this. Reopening.
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              rajeshtaneja Rajesh Taneja added a comment -

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

              Show
              rajeshtaneja Rajesh Taneja added a comment - +1 for point 4 (Option to clone user and set it to true for forum (for now)).
              Hide
              poltawski 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
              poltawski 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - I wish I had some real data for testing here...
              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              skodak Petr Skoda added a comment -

              Rajesh: I do not understand what you are proposing.

              Show
              skodak Petr Skoda added a comment - Rajesh: I do not understand what you are proposing.
              Hide
              rajeshtaneja 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
              rajeshtaneja 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_frenz 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_frenz 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
              tlevi Tony Levi added a comment -

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

              Show
              tlevi Tony Levi added a comment - Petr: what would 'real data' be? Need a DB from a large site with these problems?
              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              tlevi 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
              tlevi 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
              tlevi Tony Levi added a comment -

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

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

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

              Show
              skodak Petr Skoda added a comment - - edited The patch is not ok, my point was to NOT clone in that inner loops.
              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              rajeshtaneja 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
              rajeshtaneja 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
              rajeshtaneja 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
              rajeshtaneja 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
              drex 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
              drex 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
              rajeshtaneja 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
              rajeshtaneja 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
              aolley 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
              aolley 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Hi Netspot guys - I agree we shouldn't close this - lets try and work together for a solution
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              tlevi 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
              tlevi 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
              poltawski 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
              poltawski 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
              tlevi Tony Levi added a comment -

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

              Show
              tlevi Tony Levi added a comment - No, the memory is attached to real live objects; this won't help.
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              tlevi 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
              tlevi 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              stronk7 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
              stronk7 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
              poltawski Dan Poltawski added a comment -

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

              Integrated, thanks

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

              5 users were sent post 19

              Show
              poltawski Dan Poltawski added a comment - 5 users were sent post 19
              Hide
              poltawski 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
              poltawski 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
              stronk7 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
              stronk7 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              Hide
              marina Marina Glancy added a comment -

              Linking to the MDL-46977 which states that the fields that are unset here (such as 'institution') may be used in forum conditional availability

              Show
              marina Marina Glancy added a comment - Linking to the MDL-46977 which states that the fields that are unset here (such as 'institution') may be used in forum conditional availability

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    25/Jun/12