Locking in advagg_insert_bundle_db() [#1240208]

advagg_insert_bundle_db() was taking 1400ms on a site I'm profiling. I haven't spotted a solution to the time spent in the database yet, but noticed some locking that doesn't look right.


function advagg_insert_bundle_db($files, $filetype, $bundle_md5, $root) {
  $lock_name = 'advagg_insert_bundle_db' . $bundle_md5;
  if (!lock_acquire($lock_name)) {
    lock_wait($lock_name);
    return;
  }

If the lock isn't acquired, then to me it'd only make sense to do a lock_wait() if it was then going to re-attempt to insert the bundle, but in this case it's assuming another process is handling that and we can chuck the information away, so I don't see a reason to wait before returning.

Here's a patch that just removes the lock_wait()

Comment	File	Size	Author
#2	advagg_bundle_lock.patch	631 bytes	catch
#2
	advagg_bundle_lock.patch	553 bytes	catch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

mikeytown2 CreditAttribution: mikeytown2 commented 5 August 2011 at 06:50

Status:

Needs review

» Needs work

If inserts are REALLY slow and async mode is being used (advagg_async_generation), an async request could be sent off before the row is in the database; thus resulting in a 404. But with some code changes from a couple weeks ago, this now only can happen if "advagg_aggregate_mode" is below 2. Default is 2, so this won't affect very many people.

Code to look at is in the advagg_css_js_file_builder() function; lines #2431 - 2467.

    if (!$built || $force) {
      // Generate on request?
      if (variable_get('advagg_async_generation', ADVAGG_ASYNC_GENERATION) && !$force) {
        // Build request.
        $url = _advagg_build_url($filepath . '?generator=1');
        $headers = array(
          'Host' => $_SERVER['HTTP_HOST'],
          'Connection' => 'close',
        );

        // Request file.
        if (function_exists('stream_socket_client') && function_exists('stream_select')) {
          advagg_async_connect_http_request($url, array('headers' => $headers));
        }
        else {
          // Set timeout.
          $socket_timeout = ini_set('default_socket_timeout', variable_get('advagg_socket_timeout', ADVAGG_SOCKET_TIMEOUT));
          drupal_http_request($url, $headers, 'GET');
          ini_set('default_socket_timeout', $socket_timeout);
        }

        // Return filepath if we are going to wait for the bundle to be
        // generated or if the bundle already exists.
        if (variable_get('advagg_aggregate_mode', ADVAGG_AGGREGATE_MODE) < 2 || advagg_bundle_built($filepath)) {
          $output[$filepath] = array('prefix' => $prefix, 'suffix' => $suffix, 'files' => array_map('advagg_return_true', array_flip($files)));
        }
        else {
          // Aggregate isn't built yet, send back the files that where going to
          // be in it.
          foreach ($files as $file) {
            $output[$file . $query_string] = array('prefix' => '', 'suffix' => '', 'files' => array($file . $query_string => TRUE));
          }
          $cacheable = FALSE;
          advagg_disable_page_cache();
        }
        continue;
      }

Thanks for the continual improvements to this module :) Going to mark this needs work as it should wait if "advagg_aggregate_mode" is below 2. If it's in mode 2, then it doesn't need to use lock_wait. If you don't get to this, I'll adjust your patch and commit it tomorrow; going to bed now.

Comment #2

catch

he/him

English

CreditAttribution: catch commented 5 August 2011 at 07:22

Status:

Needs work

» Needs review

File	Size
advagg_bundle_lock.patch	631 bytes

Heh was wondering about that a bit but thought I'd let you answer.

Here's a new patch, I think that's what you meant. There's still a tiny chance this could fail with the existing code, but 30 seconds is a long time (and the rest of the request has to finish as well).