Add hash column to {locales_source} to query faster locale strings

Hm. I'm not sure about that.

1. The hash is not guaranteed to be unique. As a consequence, it should not be a unique key
2. We should have both hash = :hash and source = :source

One method I would consider is to combine the partial-index feature of MySQL and the functional index feature of other database engines by making an index on (context, source(30)) and querying on:

context = :context AND SUBSTRING(source, 1, 30) = SUBSTRING(:source, 1, 30) AND source = :source

This query can be optimized by at least MySQL, PostgreSQL and SQL Server: MySQL will satisfy context = :context AND source = :source with it's partial index, PostgreSQL and SQL Server should be able to satisfy context = :context AND SUBSTRING(source, 1, 30) = SUBSTRING(:source, 1, 30) using a functional index (or what SQL Server calls an indexed computed column, but the idea is the same).

Could we have a reroll of this one?

• Change md5() with drupal_hash_base64(), and change the size of the hash table accordingly
• Add a batch-ed update function to create the hashes

Agreed about uniqueness because we have version column so same source could be stored for different versions

No, we don't keep individual sources for different versions. The version field is for keeping a record of when we last asked for a translation of the string.

I guess Damien's point is that you cannot be sure that the hash function will produce a unique string (?). There is a (theoritical) possibility that two strings will generate the same hash. If your code assumes that the hashes are unique, you might as well mark the column as unique.

+  $hashkey = drupal_hash_base64($source . $context);

I suggest we add a separator, e.g. \0, to avoid collisions, e.g. $source='foo', $context='bar' and $source='foobar', $context=''.

+      'hashkey' => array(
+        'description' => 'MD5 hash of the concatenation of context and string, used for quick lookups.',
+        'type' => 'char',
+        'length' => 64,

It is no longer an MD5 hash.
drupal_hash_base64() seems to always return a 44-character string.
I suggest we name the column just "hash" (or "hash_key", assuming it is unique).

Not sure about uniqueness - data could be migrated

I think we should kill the duplicates in the upgrade hook. Having duplicates lead to undefined results (with and without the patch). For each duplicate source we can easily delete those that have not been translated. If more than one have been translated, we should keep the one that is returned by the default SELECT query used in locale() (assuming that the query always returns the same row - if it doesn't then we can't do better than just keeping a random row).

but "hash" is reserved by sql

Hmm, the name is used in {registry_file}.

sub from #705284: Locale strings Import Performance
ref: #622500: Add hashkey to strings and translations for quicker comparison

LDO case, it compares 100 times of strings than drupal core but import strings still very fast. (3 times faster than drupal core to import same .po). it seems we still have some space can be improve.

Haven't reviewed the patch here but someone opened #1131048: Interface translation import times out during install (and there are duplicates) suggesting the installer won't complete on some hosting due to locale strings import - would this help with that?

function drupal_hash_base64($data) {
  $hash = base64_encode(hash('sha256', $data, TRUE));
  // Modify the hash so it's safe to use in URLs.
  return strtr($hash, array('+' => '-', '/' => '_', '=' => ''));
}

drupal_hash_base64 is too complex in this case ?? we only need an unique key here.

using CRC32 is another choose.
from l10n, we only have less collisions, CRC32 save more space & comparisons

my l10n test data: ~ 196310 rows
SELECT CRC32(value) as crc, COUNT(value) as total FROM l10n_server_string GROUP BY crc HAVING total > 1
result: 22 rows

** I haven't do performance test

Adding UI language translation tag.

Gabor asked me for input on this.

He asked about using md5: I don't see this as a problem since we already use this for cache keys (eg. filter cache).

I think benchmarks in this case should be done by evaluating queries directly in SQL cli since Apache/PHP add to much fluctuation.

Ok, so looks like we don't want to use drupal_hash_base64() then.

Rerolled #10. None of the recent comments have been included yet.

see #15 & #20

crc32 hash implemented. Know problem: Upgrade test fails because D7 tables have no hashkey field (yet).
For performance tests with different encriptions I suggest a few scenario's:

• Normal front page with n nodes.
• The same with locale_cache_strings set to 0
• test page with many, many translatable strings and locale_cache_strings set to 0

The first test gives a normal situation. The second test disables the short string cache. The third test puts the highest load on the database by requesting many translations.

just query for CRC32 is not safe. It should add a comparison. e.g:
SELECT lid FROM {locales_source} WHERE hashkey = :hashkey AND source = :source

theoretically, other hash functions are same but most scripts (and drupal core ?) assume it never hit collisions. ( I'm not sure )

Well, the \0 will never actually reach the database in this patch, right? So whether it is compatible with some db does not matter.

@droplet, fully agree that crc32 alone is not safe. Is was not meant to be, only meant to be an database accelerator. My criteria for a hash is 1. Database performance and 2. PHP performance. We don't need a secure or (absolute) unique result. We only need a relatively unique result to speed up the database index. I think 22 duplicates in 200k strings is unique enough. The combination with 'source' will do the trick. But performance tests may have the final answer.

@gabor, agree that "\0" does not hit the database. Additionally I checked that CRC32 produces a different result with and without this separator.

Source, context and hash included in in queries where applicable.
Patch re-rolled because of #746240: Race condition in locale() - duplicates in {locales_source}

I carried out performance tests. I have found no ground to use an hashkey for performance improvement.

The tests

Two kinds of test were performed:

1. Import a core translation.
2. Read translations from the database.

With three variations each:

• Core, without patches
• #29 patch applied
• md5 variant of #29

All tests are carried out with increasing number of records in the locales_source and locales_target database tables.

The results

1. No significant difference in performance between crc32 and md5.
2. Import is slightly slower when using an additional hashkey column.
3. Reading translations is not measurably slower when using an additional hashkey column.
Note that all measurements show a large variation is measurement results. +/- 20% is no exception! The variation is less when measuring with MySQL (test2), variation is larger when using both PHP and MySQL (test1).

For details see the attachments.

One series of test were carried out using a different patch in which the locales_source table had only an index on the 'hashkey' column. No significant performance difference was found.

    'indexes' => array(
      'hashkey' => array('hashkey'),
    ),

@andypost, your assumption is that the hashkey is unique. As @droplet showed in #15, CRC32 is not usable. It would be nice if we could test md5($source . "\0" . $context) with all source strings of l.d.o. But changes are good. This Probability table calculates the chance of two strings having the same md5 hash as less then 10e-18 (l.d.o. contains approx 350000 source strings; md5 is a 128 bit hash).

@Sutharsan,
LDO also make an assumption on the hashkey.

If we assumed md5 hashkey is unique, I think we can move hashkey into locales_target table and remove LEFT_JOIN ??

@droplet,
What assumption does l.d.o make on the hashkey?
We could do away with lid, not with LEFT JOIN. But replacing lid with hashkey would make a significant increase in data in locales_target. And less is faster.

Hashtag changed to md5, source and context removed from the queries, source_context index removed. Todo: upgrade path.

New performance tests show approximate equal results with and without hashkey. Executing t() and format_plural() with hashtag takes 2 - 6% more longer.

_locale_import_one_string_db() does use the hashkey to lookup existing source. Note that #1189184: OOP & PSR-0-ify gettext .po file parsing and generation will overhaul the import process. My bigger worry is the reduced performance of t() and format_plural() due to this change. If we can't fix that, lets drop the issue.

#43 @andypost
What kind of tests do we need?
Just ab?
or possibly just a menu callback with some kind of locale code?

My strategy was to use focused testing with minimum influence of processes we are not working on. Testing with 'realistic' situations does not give usefull information. As an example lets test the response time of the drupal front page using ab. The variation in the response time is large (StdDev > 10%), approx 50 strings are loaded through t(), translated strings are cached in t() per page call, translated strings are cached in locale() in the database. A single (un cached) call of t() takes approx 0.2 msec, 50 t() calls on the home page take approx 10 msec. Even large variations of 20% in t() execution (= 2 msec) will never be noticed in ab tests of the home page (70 - 100 msec). Testing realistic situations is not usable, we need focused testing.

I executed three kinds of tests:

1. Database performance of individual sql queries
2. Import performance
3. t() and format_plural() performance

All tests were performed with varying number of string in the database. Simulating small and medium size sites.

No 1. Tests the database performance of a set of select queries which could be executed by t().

SET profiling = 1;
SELECT s.lid, t.translation, s.version FROM locales_source s LEFT JOIN locales_target t ON ...
...
SHOW PROFILES;

No 2. Tests the execution time of the import function _locale_import_read_po(). Time measured by adding time_start() and time_read() functions before and after the function call.

No 3. Tests the execution time of a large number of t() and format_plural() function calls. Internal caching is turned off as much as possible to focus on the database retrieval. For this I have written a tTest module which you find in my Sandbox. The module calls t() or format_plural() with a fixed set of strings taken from D7 core. Locale string caching was turned off during all tests.

back to NW first,

- update.php is failed on Localize enabled.
- missing add hashkey field in locale_update_8006
- switch between patched / non-patch, it will leave some strings haven't hashkey (it also may affect benchmark result)

The version column once again is only used in locale() for lookup I believe (and is updated on use), to limit the size of the cache in memory for short strings (if you had strings leftover from previous versions). I'm not sure of the effectiveness, with Drupal releases going this far and wide (3 years inbetween them) and much more contribs used which also change a lot (but are not represented in this version tracking) I think its likely that the version tracking does not have much usefulness (and it slows down pages on major version updates as long as these updates happen for strings).

Correction: looks like the version column is also used for minor versions, in which case this might still be a good way to weed out outdated strings on a site. It is really only used for that that I know.

As I'm already aware of this: http://drupal.org/node/1189184 I know that the following code will be thrown away at some point. But as I've already written it, I'd post it here. It may show on how could batch inserts/updates be implemented in current situation.

1. I didn't write it to the end and didn't test it, so it's more of showing the idea, rather than the code for use.
2. It relies on the schema changes posted by andypost and modified on the codesprint today. We assumed, that hashcode should be the substitution for "lid" as PK.

function _locale_import_one_string_db(&$report, $langcode, $context, $source, $translation, $location, $overwrite_options, $customized = LOCALE_NOT_CUSTOMIZED, $insert_everything = FALSE) {
  function source_arr($var) {
    $keys = array('hashcode', 'location', 'source', 'context');

    $res = array();
    foreach($keys as $v){
      $res[$k] = $var[$v];
    }
    return $res;
  }
  
  function target_arr($var) {
    $keys = array('hashcode', 'translation', 'language', 'customized');

    $res = array();
    foreach($keys as $v){
      $res[$k] = $var[$v];
    }
    return $res;
  }

  
define('MAX_ALLOWED_ROWS_NUMBER', 500);

  if (!locale_string_is_safe($translation)) {
    watchdog('locale', 'Import of string "%string" was skipped because of disallowed or malformed HTML.', array('%string' => $translation), WATCHDOG_ERROR);
    $report['skips']++;
    return 0;
  }

  // Initialize overwrite options if not set.
  $overwrite_options += array(
    'not_customized' => FALSE,
    'customized' => FALSE,
  );
  
  $buffer = drupal_static(__FUNCTION__ . 'buffer', array());
  
  $buffer[] = array(
    'hashcode' => md5($source . LOCALE_PLURAL_DELIMITER . $context),
    'location' => $location,
    'source'   => $source,
    'context'  => $context,
    'translation' => $translation,
    'language' => $langcode,
    'customized' => $customized,
    'overwrite_options' => $overwrite_options,
  );

  if ((count($buffer) < MAX_ALLOWED_ROWS_NUMBER) && !$insert_everything) {
    return 1;
  }
  
  $insert_source = array();
  $insert_target = array();
  $delete_source = array();
  $delete_target = array();
  
  $keys = array();
  foreach($buffer as $buff){
    $keys[] = $buff['hashcode']; 
  }
  
  $strings = array();
  $qr = db_select('locales_source', 's');
  $qr->join('locales_target', 't', 't.hashcode = s.hashcode AND t.language = s.language');
  $qr->fields('s', array('hashcode'))
     ->fields('t', array('customized'));
  $qr->condition('t.hashcode', $keys, 'IN');   
  $qr->condition('t.language', $language, '=');   
  $result = $qr->execute();
  while($row = $result->fetchAssoc()) {
    $strings[$row['hashcode']] = $row['customized'];
  }   
  
  foreach($buffer as $buff){
    if (!empty($translation)) {
      // Skip this string unless it passes a check for dangerous code.
      //if (isset($string->lid))
      if (isset($strings[$buff['hashcode']])) {
        // We have this source string saved already.
        $delete_source[] = $buff['hashcode'];
        $insert_source[] = source_arr($buff);
  
        //if (!isset($string->customized))
        if (empty($strings[$buff['hashcode']])) {
          // No translation in this language.
          $insert_target[] = $buff;
          
          $report['additions']++;
        }
        elseif ($buff['overwrite_options'][$strings[$buff['hashcode']] ? 'customized' : 'not_customized']) {
          // Translation exists, only overwrite if instructed.
          $delete_target[] = $buff['hashcode'];
          $insert_target[] = target_arr($buff);
          
          $report['updates']++;
        }
      }
      else {
        // No such source string in the database yet.
        $insert_source[] = source_arr($buff);
        $insert_target[] = target_arr($buff);

        $report['additions']++;
      }
    }
    elseif (isset($string->lid) && isset($string->customized) && $overwrite_options[$string->customized ? 'customized' : 'not_customized']) {
      // Empty translation, remove existing if instructed.
      $delete_target[] = $buff['hashcode'];      

      $report['deletes']++;
    }
  }

  db_delete('locales_target')
    ->condition('hashcode', $delete_target, 'IN')
    ->condition('language', $langcode)
    ->execute();
    
  db_delete('locales_source')
    ->condition('hashcode', $delete_source, 'IN')
    ->execute();
 
  
  db_insert('locales_source')
    ->fields(array('hashcode', 'location', 'source', 'context'))
    ->values($insert_source)
    ->execute();
    
  db_insert('locales_target')
    ->fields(array('hashcode', 'translation', 'language', 'customized'))
    ->values($insert_target)
    ->execute();

  $buffer = array();
}

P.S. As far as I'm concerned for now db_merge can't make batch updates that's why I decided to make insert+delete instead of 1 update.
P.P.S. The motivation for this code comes from here: http://drupal.org/node/361597#comment-5899520

42: 851362-locale-hashkey-42.patch queued for re-testing.

re-rolled the patch.

ignore the previous comment.

I will rerrol this

rerrol with no conflicts

OK how does this change the performance of imports and lookups? @Sutharsan in #25 explained a strategy but did not explain the results AFAIS. Need performance results.

Bumping to the 8.2.x branch.

Note that #65 is still using the old array notation.