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

        1. sc.php
          0.5 kB
          Ankit Agarwal

          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