See whether we can ditch the node_load from the editablefields formatter. In a view with 100 rows and 1 editablefield formatter means that we will have 100 node_loads...

Comments

vishun’s picture

Assigned: vishun » andreiashu

This does appear to be the case and I have a working example where page load goes from ~2.5 seconds to ~10 seconds with editablefields involved. In reviewing the code for theme_editablefields_formatter_editable() in editablefields/editablefields.module it seems like $element['#node'] used to be used but has since been replaced with a node_load due to some revision handling.

Is there any way you might be able to explain the scenario that this code accounts for?

<?php
//  $node = $element['#node'];
  $revision = !empty($element['#node']->vid) ? $element['#node']->vid : NULL;
  $node = node_load($element['#node']->nid, $revision);
?>

With the latter two lines replaced in favor of the first line, performance is indeed significantly improved, making page load time go from ~2.5 seconds to ~3.5 seconds. Also, based on the code it seems that if the formatter is set to editable_html that a 2nd node_load will occur, perhaps previously required with the $element['#node'] approach. Outside of the editable_html scenario, the $node object appears to only be used for node_access() and $node->nid attribute on the div.

It appears as though $element['#node'] has almost everything that's needed, except $element['#node']->format for node_access()'s filter_access() check which is specifically used for $op 'update' and checks to see if the node's body is set to a format that the user has access to. One solution for skipping the node_load and cutting down overhead is doing a database query for the missing 'format' key.

<?php
  $node = $element['#node'];
  $node->format = db_result(db_query("SELECT format FROM {node_revisions} WHERE vid=%d", $node->vid));
  //$revision = !empty($element['#node']->vid) ? $element['#node']->vid : NULL;
  //$node = node_load($element['#node']->nid, $revision);
?>

These changes have made page load times go from ~2.5 seconds to ~5 seconds. I know this measurement is pretty much unique to this specific setup, but it's allowed me to gauge that 5 seconds have been shed. Assuming of course that this may be missing some logic for something revision related.

vishun’s picture

I have two sites that are utilizing the D6 version of this module and if there isn't anyone dealing with D6 issues anymore, I may be willing to try take them on and address them. Any chance of becoming a co-maintainer?

andreiashu’s picture

Please send an email to markfoodyburton about co-maintaining this module.
Sorry, at the moment I won't be able to review patches for this module because of a lack of time.

markfoodyburton’s picture

Vishun, patching this would be great (I'm using editable fields on a d6 too, but dont have much time right now)
I'm happy to add you as a co-maintainer (done)

Cheers

Mark.

markfoodyburton’s picture

Assigned: andreiashu » vishun
vishun’s picture

Awesome! thanks so much guys, really appreciate it.

Any ideas why the proposed solution above may not be feasible? From what I can tell, the $node->vid should always be the present and active revision and the only scenario I can picture editablefields needing to work with a different revision than what is current is if editablefields was designed to work with specific revisions of a node. I believe $node->vid should never be empty even with revisioning off, so I'm just trying to understand what scenario the node_load is accounting for before ultimately taking it out of play.

pdrake’s picture

Unnecessary node_loads can be avoided by loading the node using only the nid if the revision is the current revision. This takes advantage of the node_load static node cache, which should have the node in question pre-loaded in cases where the current revision is being edited. The attached file accomplishes this with a single query to the {node} table to determine if the loaded node revision is current.

rjbrown99’s picture

Here's where the node_load came from and why: http://drupal.org/node/488816#comment-2046882

rjbrown99’s picture

Status: Active » Needs review

Patch here, changing status.

pdrake’s picture

@rjbrown99 makes sense. The patch in #7 retains the node_load, but allows the use of statically cached node if applicable.

webunion’s picture

what about a patch for version 7? i'm in need of this urgently.

vishun’s picture

Same solution, just a little compressed.

Does anyone see a reason why we need this?

  if (content_handle('widget', 'multiple values', $field) == CONTENT_HANDLE_CORE) {
  }
vishun’s picture

Doh. Previous patch was generated using the local repository which had the previously proposed change as opposed to the difference between the original module and the newly proposed changed. The following patch should work properly for a stock version of the module while the former likely will not.

EDIT: I'm a moron and forgot to change the paths in this reposting. This probably means the patch needs to be applied from your Drupal root as opposed to the module directory. So sorry.

pdrake’s picture

Status: Needs review » Needs work

The patch in #13 will execute a query whether it is required or not (ie instances where the revision is already NULL).

vishun’s picture

In what scenarios will there be no vid provided? Please provide enough information to adequately refute the vid portion of #6.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Closed (outdated)

Last update was 2012, going to close as outdated because it's against 6.x branch.