Reduce memory footprint

Hi Sutharsan,

Thanks for your post, yes this is a good idea. The Scheduler codebase has expanded considerably over the last 9 months as Pieter and I have been working on many enhancements and new features.

I will go through and make a specific list of suggestions of functions to move, for review. Given that the 7.x-1.2 release is very close, I expect we will not make such major structural changes until after, to give us much more exposure of time and usage during the next -dev phase to discover any problems.

Jonathan

I've made a start on this.

scheduler.cron.inc

This file contains functions which are only required during a cron run. I've moved into it the following functions:

_scheduler_publish()
_scheduler_unpublish()
_scheduler_scheduler_nid_list()
_scheduler_scheduler_api()
_scheduler_run_cron() - called from hook_menu [and was called from admin settings form]

The file is added using require_once('scheduler.cron.inc') in scheduler_cron(). Is that an acceptable way to include it? The location will always be in the same folder, so I do not think we need to construct the full path. This new files totals 242 lines which is 14% of scheduler.module - I know that number of lines is not exactly the same a the memory taken up by the functions, but it does give an indication.

scheduler.admin.inc

This can contain functions which are only used by the admin, ie someone who has access to the Scheduler config settings. The functions moved here are

scheduler_admin() - the main settings form
scheduler_admin_validate()
scheduler_admin_submit()
_scheduler_lightweight_cron() - the lightweight tab on the form
_scheduler_lightweight_cron_submit() [new]
_scheduler_generate_key()
_scheduler_timecheck()
theme_scheduler_timecheck()

This file is included by adding 'file' => 'scheduler.admin.inc' into hook_menu() for the following paths:

'admin/config/content/scheduler'
'admin/config/content/scheduler/default'
'admin/config/content/scheduler/cron'
'admin/config/content/scheduler/timecheck'

_scheduler_lightweight_cron_submit() is a new two-line submit handler which loads the new scheduler.cron.inc file then calls _scheduler_run_cron(). I could not find a way to define an extra file to be added when the submit button is pressed, but there might be a way. This new file is also 240 lines so far = 14% of scheduler.module

Leave in scheduler.module

I don't think we can move any hook implementations as these might be called from anywhere. The following should therefore stay where they are:

scheduler_permission()
scheduler_menu()
scheduler_list_access_callback() used directly in hook_menu
_scheduler_cron_access() used directly in hook_menu
scheduler_help()
_scheduler_use_date_popup() used in scheduler_form_alter(), scheduler_date_value_callback() and _scheduler_strtotime()
scheduler_form_node_type_form_alter()
scheduler_form_alter()
scheduler_list() used by admin, but also by user if they have 'Schedule nodes' permission
_scheduler_delete_row_confirm() - called from scheduler_list()
_scheduler_delete_row_confirm_submit()  - called from scheduler_list()
_scheduler_allow() used when editing a node and during cron
_scheduler_strtotime during node edit 
_scheduler_strptime - only called from _scheduler_strtotime
scheduler_cron_is_running()

I've tested these changes manually and also using the automated tests. All OK so far and .module is reduced to 73% of its previous size. But I think there may be more functions that could be moved out. Please take a look at the above and correct me, or make further suggestions. If you would like a patch let me know, but I thought it would be worth posting my findings first.

Jonathan

Thanks for the feedback. I think the other main area where we could save on is the the functions required during node edit, as this will not be needed for the majority of site visitors. scheduler_form_alter() and a few subsidiary functions accounts for another 200 lines which is 11% of scheduler.module. However, I am not sure how we would go about including the scheduler.edit.inc file? Could it be done with hook_menu_alter detefting a node/n/edit page? Or should we have the normal hook_form_alter in the main .module just as a stub, and use it to include and call the 'helper' form alter?

I think it is helpful to try to keep the separate files oprganised by activity type. Then it should be reasonably clear what they contain and where they might be needed. Do you agree with that?

To answer my own question I just tried the second idea, having our implementation of hook_form_alter() in the main .module but all the work is moved to a new file.

function scheduler_form_alter(&$form, $form_state) {
  module_load_include('inc', 'scheduler', 'scheduler.node_edit');
  _scheduler_form_alter(&$form, $form_state);
}

and this seems to work ok having done a very quick manual test and the full automated testing.

Yes, since posting I realised that there should be some conditional checks. The obvious ones are if it is a node edit form and the user has permssion, as these are already at the top of the existing scheduler_form_alter()

function scheduler_form_alter(&$form, $form_state) {
  // Is this a node form and does the user have permission to use Scheduler.
  if (!empty($form['#node_edit_form']) && user_access('schedule (un)publishing of nodes')) {
    module_load_include('inc', 'scheduler', 'scheduler.edit');
    _scheduler_form_alter(&$form, $form_state);
  }
}

The next lines in the original check if scheduled publishing and/or unpublishing has been enabled for this node type.

$publishing_enabled = variable_get('scheduler_publish_enable_' . $form['type']['#value'], 0) == 1;
$unpublishing_enabled = variable_get('scheduler_unpublish_enable_' . $form['type']['#value'], 0) == 1;

I think it is worth checking these too here, even though those lines will need to be repeated in _scheduler_form_alter()

Thanks for the feedback. With these three .inc files the size of scheduler.module reduces from 1760 to 1090 ie down to 62% of the original. I will go through the remainder of the functions just to see if there is any more we can move, but I think having three new files is probably about the right balance.

It's quite readable - the lines are taken exactly from the original hook_form_alter()

/**
 * Implements hook_form_alter().
 */
function scheduler_form_alter(&$form, $form_state) {
  // Is this a node form and does the user have permission to use Scheduler.
  if (!empty($form['#node_edit_form']) && user_access('schedule (un)publishing of nodes')) {
    // Check if scheduling has been enabled for this node type.
    $publishing_enabled = variable_get('scheduler_publish_enable_' . $form['type']['#value'], 0) == 1;
    $unpublishing_enabled = variable_get('scheduler_unpublish_enable_' . $form['type']['#value'], 0) == 1;
    if ($publishing_enabled || $unpublishing_enabled) {
      module_load_include('inc', 'scheduler', 'scheduler.edit');
      _scheduler_form_alter(&$form, $form_state);
    }
  }
}

Here's a first-draft patch just to see if the D.O. auto-testing behaves in the same way as my local simpletests.

I can see that in the function above it should have been
_scheduler_form_alter($form, $form_state); without the &
That was not detected in my local simpletest runs.

That fixed the the warning, but the fatal error is still there. OK, so the 'scheduler.cron.inc' file appears not get loaded by the module_load_include('inc', 'scheduler', 'scheduler.cron') in scheduler_cron(). But it works fine in my local simpletest runs.

The admin.inc file does get loaded during D.O. testing via the 'file' addition in hook_menu(). The module_load_include() in hook_form_alter does not cause any error, but maybe the test terminates before that part. So how does module_load_include() differ when run on D.O. testing compared to my local simpletests. Is it that the file is located in a different directory relative to the module?

Just checked the test log.

[09:47:16] Command [git apply -v -p1 /var/lib/drupaltestbot/sites/default/files/review/2170353_11.reduce_memory_footprint.patch 2>&1] succeeded.
[09:47:16] Applied [2170353_11.reduce_memory_footprint.patch] to:
 > scheduler.module
[09:47:16] Invoking operation [syntax]...
[09:47:16] Command [php -l -f './sites/default/modules/scheduler/scheduler.module' 2>&1] succeeded.

There is no mention of the three new files, so maybe the patch to create them is not working?

I had not noticed that even though I ran git diff scheduler.cron.inc >> 2170353_11.reduce_memory_footprint.patch the new file was not added to the patch because it was not being tracked by git. Here's a patch which does include the three new files.

Hopefully this is the correct format for the patch. For the previous one I had done

git add -N scheduler.admin.inc
git diff scheduler.admin.inc  >> the.patch

but this creates a patch from an empty file which exists.
whereas I think it should have been just

git diff /dev/null scheduler.admin.inc  >> the.patch

so that when applying the patch it knows that the original file does not exist yet.

Fixed grammar in the summary

The original contents of this comment are now in the issue summary, to allow easier updating and comparing as work progresses.

The patch doesn't apply any more. There have been many commits in the meanwhile so rerolling this will be quite a lot of work.

This is very valuable though, especially moving out the administration forms which are rarely used is a big win. A drawback is that if we move huge chunks of code around all the patches in the queue will need to be rerolled. Maybe we should do a little sprint first towards the next release, clear out most of the open issues, and then tackle this again.

Thanks for the feedback and I'm pleased you think the idea is worthwhile. Yes, I realised that the patch would very quickly become non-applyable (non-applicable?) so I left this thread and did not do any more on it after my initial testing and proof of concept, and waited for someone to give an opinion on work done so far.

Can we get all the existing patches/bugs fixed, then work on this one as the final patch before releasing 1.3? This would have the advantage that 1.3 had the improved memory fpotprint, and we could then be free to start any work on the post-release dev version without the complication of this issue needing to be done first.

The list of issues to consider is shown on #2255165: Enhancements for 7.x-1.3

Yes I think that it a sound plan. I'll make some time available in the coming weeks to review all the listed issues. It would be nice to have a new release before Drupalcon Amsterdam.

I have added a Next release 7.x-1.3 section on the project page, and created a shortcut to show 7.x issues with 'needs work' or 'needs review' status to help us. There are currently only six issues in this list, so unless users report lots of new bugs, we should have a good chance of clearing this down.

It took a bit longer than planned, but we have worked our way through all issues that were in progress.

Taking the existing code and splitting out as listed in #19 is the first task to do. I can start on this, if you have not done so already?

Would be great if you can start on this. Looking at the list from #19, especially scheduler.admin.inc and scheduler.cron.inc are interesting. They are the least used parts and provide the largest gains.

scheduler.edit.inc is also a nice target if you have some time left, but I wouldn't delegate scheduler_form_alter() to it - this hook is likely to fire on most page requests, so that would mean that this file gets included almost all the time, and that would negate any gains we could get.

I'm at Drupalcamp Ghent this weekend so I'm quite busy, but I will check the queue regularly. If I can't make it this weekend I will definitely have time on Tuesday to review.

Yes, you are right that scheduler_form_alter() should remain in .module - the notes above were maybe not clear, but in #9 you'll see that I already thought of that. The bulk of the function is moved to .edit.inc and only called when required.

I have moved the complete listing of functions from #19 into the issue header and added a few notes to make it clearer about hook_form_alter. We have some new functions which will need to go somewhere, so I'll update the issue summary with the latest position after re-working the code.

Here is a patch for 7.x-1.2+22 - all code changes have been done from the new code-base but I have compared against the previous patch, to check my work. I've updated the issue summary. The split is currently identical to what we had before, apart from two new functions which have gone into admin.inc.

The basic functionality works via manual testing, but two out of the 401 automated tests fail. I will look into this now, but thought you may like to know the progress so far.

The problem was caused by _scheduler_scheduler_api() which had been moved into .cron.inc. However, due to the enhancement we made which catered for 'publish immediately' when entering a date in the past, this function is now also called in scheduler_node_presave ie during normal editting. The simple solution is to move _scheduler_scheduler_api() back to the .module file. Luckily this is a small function so has a minimal effect on the space and memory savings.

Just did a bit more testing and the scheduled content operations need to be moved to admin.inc. There was also a missing 'file' value for the /scheduler/delete/% url. This was not picked up by automated testing, as this functionality is not covered. However, we can follow this up in the separate issue #2112869: 7.x test coverage

This looks great. Do you think it would be wise to put this in immediately after creating a new release? That way we reduce the risk of this huge code change breaking something unexpected. The queue is empty now so we could hold off any other changes before the release, and committing this straight after the release would save us from another reroll.

This big change can then sit for a few months in 7.x-1.x - if something is broken chances are good it will be discovered before the following release.

Much as I would like to have this improvement in 7.x-1.3 I agree that we should leave it until after the release. It is a very large change, and I'm not completely convinced I have tested everything that's not covered by the automated testing. It is tempting to take the risk that it is all correct, but the downside is that we break customers' sites and that would be worse than the memory improvement we gain.

Also I agree that we should make no more patches or changes until after this is committed, as the review queue is now empty.

Found another problem - scheduler_date_popup_pre_validate_alter() has been moved to .edit.inc but during validation this function is not known about and does not get called. Hence the default time is not added and date module gives an error saying the input is not in the correct format. This can be resolved by moving scheduler_date_popup_pre_validate_alter() back into .module or maybe an alternative would be to ensure .edit.inc is included from scheduler_node_validate() ?

We need to work out why this was not trapped in automated testing as we do have tests to check the default time is added for both textfield entry and date-popup entry. Not re-rolling a patch yet, just recording that this error exists.

OK that is something we should fix before getting this in. We should also check the test coverage.

Here's a patch against 7.x-1.3 moving scheduler_date_popup_pre_validate_alter() back to .module
I have started some discussion on #2112869: 7.x test coverage as to why the automated tests may have passed when manual testing failed. I'd like to commit these large-scale changes to dev soon, even if we need a follow-up patch or two. Then we'll have a better chance of finding the bugs in our normal regular use of the development code.

Test failure due to #2375345: Dependency on core module results in PHP Fatal error: Cannot redeclare system_requirements()

Alright, let's do this! This is blocking all other work at the moment. I have reviewed the patch and made some small documentation fixes so the docblocks in the scheduler.admin.inc file conform to the API documentation and comment standards. Attaching the interdiff.

This is committed in 7.x-1.x, thanks a lot Jonathan!!

I was actually writing a comment saying exactly the same thing, that it is blocking other patches, and we'll have more chance to discover any problems if this is committed and in use.

Thanks for the correcting and improving the docblocks.

I've updated the summary with how the files get loaded, so that we have a complete reference of the changes.