Ticket #326: avoid_string_operations.patch

chert_postlist.h

@@ -83 +83 @@
                 Xapian::docid did, bool adding,
                 PostlistChunkReader ** from, PostlistChunkWriter **to);
 
+#define KEYBUF_MAX_LEN CHERT_BTREE_MAX_KEY_LEN
+        mutable char keybuf[KEYBUF_MAX_LEN];
+        mutable size_t keybuf_curlen;
+
         /// Compose a key from a termname and docid.
-        static string make_key(const string & term, Xapian::docid did) {
-            string key = make_key(term);
-            key += pack_uint_preserving_sort(did);
-            return key;
+        void make_key(const string & term, Xapian::docid did) const {
+            keybuf_curlen = 0;
+            make_key(term);
+            keybuf_curlen += append_packed_uint_preserving_sort(
+                keybuf + keybuf_curlen, KEYBUF_MAX_LEN - keybuf_curlen, did);
         }
 
         /// Compose a key from a termname.
-        static string make_key(const string & term) {
+        void make_key(const string & term) const {
             // Special case for doclen lists.
-            if (term.empty()) return string("\x00\xe0", 2);
+            if (term.empty()) {
+                keybuf[0] = '\x00';
+                keybuf[1] = '\xe0';
+                keybuf_curlen = 2;
+                return;
+            }
 
-            return pack_string_preserving_sort(term);
+            keybuf_curlen = append_string_preserving_sort(keybuf, KEYBUF_MAX_LEN, term);
         }
 
         bool term_exists(const string & term) const {
-            return key_exists(make_key(term));
+            make_key(term);
+            return key_exists(keybuf, keybuf_curlen);
         }
 
         /** Returns number of docs indexed by @a term.
@@ -129 +140 @@
          */
         Xapian::Internal::RefCntPtr<const ChertDatabase> this_db;
 
+        ChertPostListTable * pltable;
+
         /// The termname for this postlist.
         string tname;
 

chert_table.h

@@ -227 +227 @@
         memmove(p + I2 + K1, key_.data(), key_len);
         set_component_of(1);
     }
+    void form_key(const char * key_, size_t keylen) {
+        if (keylen > CHERT_BTREE_MAX_KEY_LEN) {
+            // We check term length when a term is added to a document but
+            // chert doubles zero bytes, so this can still happen for terms
+            // which contain one or more zero bytes.
+            std::string msg("Key too long: length was ");
+            msg += om_tostring(keylen);
+            msg += " bytes, maximum length of a key is "
+                   STRINGIZE(CHERT_BTREE_MAX_KEY_LEN) " bytes";
+            throw Xapian::InvalidArgumentError(msg);
+        }
+
+        set_key_len(keylen + K1 + C2);
+        memmove(p + I2 + K1, key_, keylen);
+        set_component_of(1);
+    }
     // FIXME passing cd here is icky
     void set_tag(int cd, const char *start, int len, bool compressed) {
         memmove(p + cd, start, len);
@@ -396 +412 @@
          *          false if key is not found in table.
          */
         bool get_exact_entry(const std::string & key, std::string & tag) const;
+        bool get_exact_entry(const char * key, size_t keylen, string & tag) const;
 
         /** Check if a key exists in the Btree.
          *
@@ -409 +426 @@
          *          false if key is not found in table.
          */
         bool key_exists(const std::string &key) const;
+        bool key_exists(const char * key, size_t keylen) const;
 
         /** Read the tag value for the key pointed to by cursor C_.
          *
@@ -597 +615 @@
         void read_root();
         void split_root(uint4 split_n);
         void form_key(const std::string & key) const;
+        void form_key(const char * key, size_t keylen) const;
 
         char other_base_letter() const {
            return (base_letter == 'A') ? 'B' : 'A';

chert_positionlist.cc

@@ -70 +70 @@
               did << ", " << term);
 
     string data;
-    if (!get_exact_entry(pack_uint_preserving_sort(did) + term, data)) {
+    string key;
+    // 5 bytes is usually enough for a packed uint
+    key.reserve(5 + term.size());
+    append_packed_uint_preserving_sort(key, did);
+    key.append(term);
+    if (!get_exact_entry(key, data)) {
         // There's no positional information for this term.
         return 0;
     }
@@ -106 +111 @@
     positions.clear();
 
     string data;
-    if (!table->get_exact_entry(pack_uint_preserving_sort(did) + tname, data)) {
+    string key;
+    // 5 bytes is usually enough for a packed uint
+    key.reserve(5 + tname.size());
+    append_packed_uint_preserving_sort(key, did);
+    key.append(tname);
+    if (!table->get_exact_entry(key, data)) {
         // There's no positional information for this term.
         current_pos = positions.begin();
         return false;

chert_positionlist.h

@@ -34 +34 @@
 
 class ChertPositionListTable : public ChertTable {
     static string make_key(Xapian::docid did, const string & tname) {
-        return pack_uint_preserving_sort(did) + tname;
+        string result;
+        // 5 bytes is usually enough for a packed uint
+        result.reserve(5 + tname.size());
+        append_packed_uint_preserving_sort(result, did);
+        result.append(tname);
+        return result + tname;
     }
 
   public:

chert_utils.h

@@ -26 +26 @@
 #include "omassert.h"
 
 #include <xapian/types.h>
+#include <xapian/error.h>
 
 #include <string>
 
@@ -224 +225 @@
  *  of 256 bytes on the length of the integer.  However, this is unlikely to
  *  ever be a problem.
  *
+ *  @param result A string to append the representation of the integer to.
  *  @param value  The integer to represent.
- *
- *  @result       A string containing the representation of the integer.
  */
 template<class T>
-string
-pack_uint_preserving_sort(T value)
+void
+append_packed_uint_preserving_sort(string & result, T value)
 {
     // Check unsigned
     STATIC_ASSERT_UNSIGNED_TYPE(T);
+    STATIC_ASSERT(sizeof(T) >= 4);
 
-    string result;
+    string::size_type start = result.size();
+    if (value <= 0xffff) {
+        if (value < 0xff) {
+            // 1 byte
+            char buf[2];
+            buf[0] = char(1);
+            buf[1] = char(value & 0xff);
+            result.append(buf, 2);
+            return;
+        } else {
+            // 2 bytes
+            char buf[3];
+            buf[0] = char(2);
+            buf[1] = char((value >> 8) & 0xff);
+            buf[2] = char(value & 0xff);
+            result.append(buf, 3);
+            return;
+        }
+    }
+    if (value <= 0xffffff) {
+        // 3 bytes
+        char buf[4];
+        buf[0] = char(3);
+        buf[1] = char((value >> 16) & 0xff);
+        buf[2] = char((value >> 8) & 0xff);
+        buf[3] = char(value & 0xff);
+        result.append(buf, 4);
+        return;
+    }
+
+    if (value <= 0xffffffff) {
+        // 4 bytes.
+        char buf[5];
+        buf[0] = char(4);
+        buf[1] = char((value >> 24) & 0xff);
+        buf[2] = char((value >> 16) & 0xff);
+        buf[3] = char((value >> 8) & 0xff);
+        buf[4] = char(value & 0xff);
+        result.append(buf, 5);
+        return;
+    }
+
+    if (sizeof(T) > 4) {
+        if (result.capacity() < start + 9) {
+            result.reserve(start + 9);
+        }
+        // More than 4 bytes. Do first 4, then loop.
+        result.append(string::size_type(1u), char((value >> 24) & 0xff));
+        result.append(string::size_type(1u), char((value >> 16) & 0xff));
+        result.append(string::size_type(1u), char((value >> 8) & 0xff));
+        result.append(string::size_type(1u), char(value & 0xff));
+        value = value >> 16;
+        value = value >> 16;
+        while (value != 0) {
+            om_byte part = static_cast<om_byte>(value & 0xff);
+            value = value >> 8;
+            result.insert(start, 1u, char(part));
+        }
+        result.insert(start, 1u, char(result.size() - start));
+    }
+}
+
+template<class T>
+size_t
+append_packed_uint_preserving_sort(char * buf, size_t buflen, T value)
+{
+    // Check unsigned
+    STATIC_ASSERT_UNSIGNED_TYPE(T);
+    STATIC_ASSERT(sizeof(T) >= 4);
+
+    if (value <= 0xffff) {
+        if (value < 0xff) {
+            if (buflen < 2)
+                throw Xapian::InvalidArgumentError("Buffer not long enough to hold 2 byte varint.");
+            buf[0] = char(1);
+            buf[1] = char(value & 0xff);
+            return 2;
+        } else {
+            if (buflen < 3)
+                throw Xapian::InvalidArgumentError("Buffer not long enough to hold 3 byte varint.");
+            buf[0] = char(2);
+            buf[1] = char((value >> 8) & 0xff);
+            buf[2] = char(value & 0xff);
+            return 3;
+        }
+    }
+    if (value <= 0xffffff) {
+        if (buflen < 4)
+            throw Xapian::InvalidArgumentError("Buffer not long enough to hold 4 byte varint.");
+        buf[0] = char(3);
+        buf[1] = char((value >> 16) & 0xff);
+        buf[2] = char((value >> 8) & 0xff);
+        buf[3] = char(value & 0xff);
+        return 4;
+    }
+
+    if (value <= 0xffffffff) {
+        if (buflen < 5)
+            throw Xapian::InvalidArgumentError("Buffer not long enough to hold 5 byte varint.");
+        buf[0] = char(4);
+        buf[1] = char((value >> 24) & 0xff);
+        buf[2] = char((value >> 16) & 0xff);
+        buf[3] = char((value >> 8) & 0xff);
+        buf[4] = char(value & 0xff);
+        return 5;
+    }
+
+    // More than 4 bytes. Do first 4, then loop.
+    if (buflen < 6)
+        throw Xapian::InvalidArgumentError("Buffer not long enough to hold varint.");
+    buf[1] = char((value >> 24) & 0xff);
+    buf[2] = char((value >> 16) & 0xff);
+    buf[3] = char((value >> 8) & 0xff);
+    buf[4] = char(value & 0xff);
+    value = value >> 16;
+    value = value >> 16;
+    size_t bytes = 4;
     while (value != 0) {
+        ++bytes;
+        if (buflen < bytes + 1)
+            throw Xapian::InvalidArgumentError("Buffer not long enough to hold varint.");
         om_byte part = static_cast<om_byte>(value & 0xff);
         value = value >> 8;
-        result.insert(string::size_type(0), 1u, char(part));
+        buf[bytes] = char(part);
     }
-    result.insert(string::size_type(0), 1u, char(result.size()));
-    return result;
+    buf[0] = char(bytes);
 }
 
 /** Unpack a unsigned integer, store in sort preserving order.
@@ -343 +462 @@
     return value + '\0'; // Note - next byte mustn't be '\xff'...
 }
 
+inline size_t
+append_string_preserving_sort(char * buf, size_t buflen, string value)
+{
+    string::size_type i, j;
+    Assert(buflen >= 2);
+    char * pos = buf;
+    // bufend is the end of the part of buf which the string goes in.
+    const char * bufend = buf + buflen - 2;
+    j = value.size();
+    for (i = 0; i != j; ++i) {
+        if (pos == bufend)
+            throw Xapian::InvalidArgumentError("Buffer not long enough to hold string.");
+        *pos = value[i];
+        ++pos;
+        if (value[i] == 0) {
+            if (pos == bufend)
+                throw Xapian::InvalidArgumentError("Buffer not long enough to hold string.");
+            *pos = '\xff';
+            ++pos;
+        }
+    }
+    pos[0] = '\0';
+    pos[1] = '\0';
+    return (pos - buf) + 2;
+}
+
 inline bool
 unpack_string_preserving_sort(const char ** src,
                               const char * src_end,
@@ -401 +546 @@
 inline string
 chert_docid_to_key(Xapian::docid did)
 {
-    return pack_uint_preserving_sort(did);
+    string result;
+    append_packed_uint_preserving_sort(result, did);
+    return result;
 }
 
 #endif /* OM_HGUARD_CHERT_UTILS_H */

chert_cursor.h

@@ -202 +202 @@
          *          otherwise.
          */
         bool find_entry(const string &key);
+        bool find_entry(const char * key, size_t keylen) {
+            return find_entry(string(key, keylen));
+        }
 
         /// Position the cursor on the highest entry with key < @a key.
         void find_entry_lt(const string &key) {

chert_postlist.cc

@@ -34 +34 @@
 Xapian::doccount
 ChertPostListTable::get_termfreq(const string & term) const
 {
-    string key = make_key(term);
+    make_key(term);
     string tag;
-    if (!get_exact_entry(key, tag)) return 0;
+    if (!get_exact_entry(keybuf, keybuf_curlen, tag)) return 0;
 
     Xapian::doccount termfreq;
     const char * p = tag.data();
@@ -47 +47 @@
 Xapian::termcount
 ChertPostListTable::get_collection_freq(const string & term) const
 {
-    string key = make_key(term);
+    make_key(term);
     string tag;
-    if (!get_exact_entry(key, tag)) return 0;
+    if (!get_exact_entry(keybuf, keybuf_curlen, tag)) return 0;
 
     Xapian::termcount collfreq;
     const char * p = tag.data();
@@ -91 +91 @@
                             bool is_last_chunk_);
 
         /// Append an entry to this chunk.
-        void append(ChertTable * table, Xapian::docid did,
+        void append(ChertPostListTable * table, Xapian::docid did,
                     Xapian::termcount wdf);
 
         /// Append a block of raw entries to this chunk.
@@ -110 +110 @@
          *  with a different key to the original one, if for example the first
          *  entry has changed.
          */
-        void flush(ChertTable *table);
+        void flush(ChertPostListTable *table);
 
     private:
         string orig_key;
@@ -314 +314 @@
 }
 
 void
-PostlistChunkWriter::append(ChertTable * table, Xapian::docid did,
+PostlistChunkWriter::append(ChertPostListTable * table,
+                            Xapian::docid did,
                             Xapian::termcount wdf)
 {
     if (!started) {
@@ -331 +332 @@
             is_first_chunk = false;
             first_did = did;
             chunk.resize(0);
-            orig_key = ChertPostListTable::make_key(tname, first_did);
+            table->make_key(tname, first_did);
+            orig_key = string(table->keybuf, table->keybuf_curlen);
         } else {
             chunk.append(pack_uint(did - current_did - 1));
         }
@@ -379 +381 @@
 }
 
 void
-PostlistChunkWriter::flush(ChertTable *table)
+PostlistChunkWriter::flush(ChertPostListTable *table)
 {
     DEBUGCALL(DB, void, "PostlistChunkWriter::flush", table);
 
@@ -560 +562 @@
              * and we just have to write this one back to disk.
              */
             LOGLINE(DB, "PostlistChunkWriter::flush(): rewriting the first chunk, which still has items in it");
-            string key = ChertPostListTable::make_key(tname);
-            bool ok = table->get_exact_entry(key, tag);
+            table->make_key(tname);
+            bool ok = table->get_exact_entry(table->keybuf, table->keybuf_curlen, tag);
             (void)ok;
             Assert(ok);
             Assert(!tag.empty());
@@ -578 +580 @@
 
             tag += make_start_of_chunk(is_last_chunk, first_did, current_did);
             tag += chunk;
-            table->add(key, tag);
+            table->add(string(table->keybuf, table->keybuf_curlen), tag);
             return;
         }
 
@@ -609 +611 @@
              * Create a new tag with the correct key, and replace
              * the old one.
              */
-            new_key = ChertPostListTable::make_key(tname, first_did);
+            table->make_key(tname, first_did);
+            new_key = string(table->keybuf, table->keybuf_curlen);
             table->del(orig_key);
         } else {
             new_key = orig_key;
@@ -661 +664 @@
                              const string & tname_,
                              bool keep_reference)
         : this_db(keep_reference ? this_db_ : NULL),
+          pltable(&(this_db_->postlist_table)),
           tname(tname_),
           have_started(false),
           cursor(this_db_->postlist_table.cursor_get()),
@@ -668 +672 @@
 {
     DEBUGCALL(DB, void, "ChertPostList::ChertPostList",
               this_db_.get() << ", " << tname_ << ", " << keep_reference);
-    string key = ChertPostListTable::make_key(tname);
-    int found = cursor->find_entry(key);
+    pltable->make_key(tname);
+    int found = cursor->find_entry(pltable->keybuf, pltable->keybuf_curlen);
     if (!found) {
         LOGLINE(DB, "postlist for term not found");
         number_of_entries = 0;
@@ -823 +827 @@
 {
     DEBUGCALL(DB, void,
               "ChertPostList::move_to_chunk_containing", desired_did);
-    (void)cursor->find_entry(ChertPostListTable::make_key(tname, desired_did));
+    pltable->make_key(tname, desired_did);
+    (void)cursor->find_entry(pltable->keybuf, pltable->keybuf_curlen);
     Assert(!cursor->after_end());
 
     const char * keypos = cursor->current_key.data();
@@ -965 +970 @@
 {
     DEBUGCALL(DB, Xapian::docid, "ChertPostListTable::get_chunk", tname << ", " << did << ", " << adding << ", [from], [to]");
     // Get chunk containing entry
-    string key = make_key(tname, did);
+    make_key(tname, did);
 
     // Find the right chunk
     AutoPtr<ChertCursor> cursor(cursor_get());
 
-    (void)cursor->find_entry(key);
+    (void)cursor->find_entry(keybuf, keybuf_curlen);
     Assert(!cursor->after_end());
 
     const char * keypos = cursor->current_key.data();
@@ -1050 +1055 @@
     LOGVALUE(DB, doclens.size());
     if (!doclens.empty()) {
         // Ensure there's a first chunk.
-        string current_key = make_key(string());
-        if (!key_exists(current_key)) {
+        make_key(string());
+        if (!key_exists(keybuf, keybuf_curlen)) {
             LOGLINE(DB, "Adding dummy first chunk");
             string newtag = make_start_of_first_chunk(0, 0, 0);
             newtag += make_start_of_chunk(true, 0, 0);
-            add(current_key, newtag);
+            add(string(keybuf, keybuf_curlen), newtag);
         }
 
         map<Xapian::docid, Xapian::termcount>::const_iterator j;
@@ -1116 +1121 @@
             map<string, pair<Xapian::termcount_diff, Xapian::termcount_diff> >::const_iterator deltas = freq_deltas.find(tname);
             Assert(deltas != freq_deltas.end());
 
-            string current_key = make_key(tname);
+            make_key(tname);
             string tag;
-            (void)get_exact_entry(current_key, tag);
+            (void)get_exact_entry(keybuf, keybuf_curlen, tag);
 
             // Read start of first chunk to get termfreq and collfreq.
             const char *pos = tag.data();
@@ -1145 +1150 @@
                 // posting list.
                 if (islast) {
                     // Only one entry for this posting list.
-                    del(current_key);
+                    del(string(keybuf, keybuf_curlen));
                     continue;
                 }
                 AutoPtr<ChertCursor> cursor(cursor_get());
-                bool found = cursor->find_entry(current_key);
+                bool found = cursor->find_entry(string(keybuf, keybuf_curlen));
                 Assert(found);
                 if (!found) continue; // Reduce damage!
                 while (cursor->del()) {
@@ -1165 +1170 @@
             string newhdr = make_start_of_first_chunk(termfreq, collfreq, firstdid);
             newhdr += make_start_of_chunk(islast, firstdid, lastdid);
             if (pos == end) {
-                add(current_key, newhdr);
+                add(string(keybuf, keybuf_curlen), newhdr);
             } else {
                 Assert((size_t)(pos - tag.data()) <= tag.size());
                 tag.replace(0, pos - tag.data(), newhdr);
-                add(current_key, tag);
+                add(string(keybuf, keybuf_curlen), tag);
             }
         }
         map<Xapian::docid, pair<char, Xapian::termcount> >::const_iterator j;

chert_table.cc

@@ -1013 +1013 @@
     LOGCALL_VOID(DB, "ChertTable::form_key", key);
     kt.form_key(key);
 }
+void ChertTable::form_key(const char * key, size_t keylen) const
+{
+    LOGCALL_VOID(DB, "ChertTable::form_key", key);
+    kt.form_key(key, keylen);
+}
 
 /* ChertTable::add(key, tag) adds the key/tag item to the
    B-tree, replacing any existing item with the same key.
@@ -1232 +1237 @@
 }
 
 bool
+ChertTable::get_exact_entry(const char * key, size_t keylen, string & tag) const
+{
+    LOGCALL(DB, bool, "ChertTable::get_exact_entry", string(key, keylen) << ", [&tag]");
+    Assert(keylen != 0);
+
+    if (handle < 0) RETURN(false);
+
+    // An oversized key can't exist, so attempting to search for it should fail.
+    if (keylen > CHERT_BTREE_MAX_KEY_LEN) RETURN(false);
+
+    form_key(key, keylen);
+    if (!find(C)) RETURN(false);
+
+    (void)read_tag(C, &tag, false);
+    RETURN(true);
+}
+
+bool
 ChertTable::key_exists(const string &key) const
 {
     LOGCALL(DB, bool, "ChertTable::key_exists", key);
@@ -1245 +1268 @@
 }
 
 bool
+ChertTable::key_exists(const char * key, size_t keylen) const
+{
+    LOGCALL(DB, bool, "ChertTable::key_exists", string(key, keylen));
+    Assert(keylen != 0);
+
+    // An oversized key can't exist, so attempting to search for it should fail.
+    if (keylen > CHERT_BTREE_MAX_KEY_LEN) RETURN(false);
+
+    form_key(key, keylen);
+    RETURN(find(C));
+}
+
+bool
 ChertTable::read_tag(Cursor * C_, string *tag, bool keep_compressed) const
 {
     LOGCALL(DB, bool, "ChertTable::read_tag", "C_, tag, " << keep_compressed);

chert_values.h

@@ -37 +37 @@
 {
     std::string key("\0\xd8", 2);
     key += pack_uint(slot);
-    key += pack_uint_preserving_sort(did);
+    append_packed_uint_preserving_sort(key, did);
     return key;
 }