[QGIT PATCH/RFC]

Hello Marco,

While recompiling latest qgit4, I stumbled accross this. I am not quite sure you used a QLatin1String instead of a QByteArray but the attached seems to work fine...

Anyway, I'll let you decide what do to with it.

Thanks for QGit,
Abdel.

index af18fbf..2d9f415 100644

--- a/src/cache.cpp

+++ b/src/cache.cpp

@@ -70,11 +70,11 @@ bool Cache::save(const QString& gitDir, const RevFileMap& rf,

                const ShaString& sha = it.key(); 


                if (   sha == ZERO_SHA_RAW 


                    || sha == CUSTOM_SHA_RAW 

- || sha.latin1()[0] == 'A') // ALL_MERGE_FILES + rev sha

+ || sha.at(0) == 'A') // ALL_MERGE_FILES + rev sha

                        continue; 



                v.append(it.value());
-               buf.append(sha.latin1()).append('\0');
+               buf.append(sha);
                newSize += 41;
                if (newSize > bufSize) {
                        dbs("ASSERT in Cache::save, out of allocated 

space");

index ceb62fb..0d65980 100644
@@ -7,6 +7,7 @@
 #ifndef COMMON_H
 #define COMMON_H
 
+#include <QByteArray>
 #include <QColor>
 #include <QEvent>
 #include <QFont>

@@ -49,7 +50,7 @@ class QDataStream;
 class QProcess;
 class QSplitter;
 class QWidget;
-class ShaString;
+typedef QByteArray ShaString;
 
 // type shortcuts
 typedef const QString&              SCRef;

@@ -274,18 +275,6 @@ namespace QGit {
        extern const QString SCRIPT_EXT;
 }
 
-class ShaString : public QLatin1String {
-public:
-       inline ShaString() : QLatin1String(NULL) {}
-       inline ShaString(const ShaString& sha) : QLatin1String(sha.latin1()) {}
-       inline explicit ShaString(const char* sha) : QLatin1String(sha) {}
-
-       inline bool operator!=(const ShaString& o) const { return !operator==(o); }
-       inline bool operator==(const ShaString& o) const {
-
-               return (latin1() == o.latin1()) || !qstrcmp(latin1(), o.latin1());
-       }
-};
 
 class Rev {
        // prevent implicit C++ compiler defaults

index 177b24a..afa5234 100644
@@ -725,7 +725,7 @@ const Rev* Git::revLookup(SCRef sha, const FileHistory* fh) const {
 const Rev* Git::revLookup(const ShaString& sha, const FileHistory* fh) 

const {

        const RevMap& r = (fh ? fh->revs : revData->revs);

- return (sha.latin1() ? r.value(sha) : NULL);
+ return (sha.isEmpty() ? NULL : r.value(sha));
}

bool Git::run(SCRef runCmd, QString* runOutput, QObject* receiver,
SCRef buf) {

index 80c2551..2960c36 100644
@@ -95,7 +95,7 @@ static inline uint hexVal(const uchar* ch) {
 
 uint qHash(const ShaString& s) { // fast path, called 6-7 times per 

revision

- const uchar* ch = reinterpret_cast<const uchar*>(s.latin1());
+ const uchar* ch = reinterpret_cast<const uchar*>(s.data());
return (hexVal(ch ) << 24)
+ (hexVal(ch + 2) << 20)
+ (hexVal(ch + 4) << 16)

Hi Abdel,

Unfortunatly I cannot say the same here ;-)

...... cut ......

Function:

uint qHash(const QByteArray&);

is already defined in the Qt Core libraries, so I have a link error with your patch.

BTW I don't think I have understood the reason of your patch. Do you have a compile error or something ?

Thanks

Marco

Weird... it links just fine here... anyway this can be solved by renaming your version. Or just using the Qt version if that does the same thing ;-)

No, I had some warnings so I looked at the code and I just thought that QLatin1String was not appropriate here and overkill. And QByteArray should be faster...

Anyway, this was just FYI, I don't think this patch is important at all :-)

Abdel.

By the way, this function of yours is not used anywhere AFAICS.

Abdel.

Ok, now I understand that this is used by QHash, sorry.

I have gcc-4.4.1 so maybe that's the reason why linking works in my case. But I don't which version of the qhash() function does take precedence...

Abdel.

FYI I just verified that your version does take precedence over the Qt one.

Anyway, if you like the patch, we could just inherit from QByteArray instead of typedef it so that it links with your system.

Out of curiosity I just had a look at the two versions, yours:

********************************
// definition of an optimized sha hash function
static inline uint hexVal(const uchar* ch) {
return (*ch < 64 ? *ch - 48 : *ch - 87);
}

uint qHash(const ShaString& s) { // fast path, called 6-7 times per revision

   const uchar* ch = reinterpret_cast<const uchar*>(s.data());

return (hexVal(ch ) << 24)
+ (hexVal(ch + 2) << 20)
+ (hexVal(ch + 4) << 16)
+ (hexVal(ch + 6) << 12)
+ (hexVal(ch + 8) << 8)
+ (hexVal(ch + 10) << 4)
+ hexVal(ch + 12);
}
********************************

And Qt's version:

********************************
static uint hash(const uchar *p, int n)
{
uint h = 0;
uint g;

   while (n--) {
       h = (h << 4) + *p++;
       if ((g = (h & 0xf0000000)) != 0)
           h ^= g >> 23;
       h &= ~g;

}
return h;
}

uint qHash(const QBitArray &bitArray)
{
int m = bitArray.d.size() - 1;
uint result = hash(reinterpret_cast<const uchar
*>(bitArray.d.constData()), qMax(0, m));

// deal with the last 0 to 7 bits manually, because we can't trust that // the padding is initialized to 0 in bitArray.d
int n = bitArray.size();
if (n & 0x7)

       result = ((result << 4) + bitArray.d.at(m)) & ((1 << n) - 1);

return result;
}

********************************

I recompiled qgit with the Qt version and I didn't notice any performance problem with a big repo (Qt).

Just tell me if this is not interesting to you and I'll shut up :-)

Abdel.

In git we don't need to compute hashes of sha strings because they are already hashed !

That's the idea of using a custom hashing function that does nothing but taking the first chars of the sha string. Instead the general purpose Qt hashing must do real work because it has to work for any string.

When I tested I _found_ a speed difference, but now I don't remember of how much. Be sure you have warm cache when doing the test (press F5) for few times to be sure all is in RAM.

No, it is very interesting indeed. My bad I have no time for net access today.

If QByteArray is faster then QLatin1String() we should definitely change. But if I don't remember wrong QLatin1String() is already implemented above a QByteArray and the methods that we use are inherited directly from a QByteArray, but I may be wrong. I don't have access to the sources now. I only remember that when I implemented that part it took a good amount of time and testing ;-)

Thanks
Marco

Now you tell it, it's obvious indeed :-)

Thanks,
Abdel.

Sorry, I forgot. Be sure also our custom hashing function is actually called in your binary and the compiler didn't linked the Qt one instead :-)

Yes I made sure of that (see my other mail).

Abdel.