[QGIT PATCH/RFC]

[QGIT PATCH/RFC]

[Abdelrazak Younes]

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.


diff --git a/src/cache.cpp b/src/cache.cpp 
 

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");
diff --git a/src/common.h b/src/common.h
index ceb62fb..0d65980 100644
--- a/src/common.h
+++ b/src/common.h
@@ -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
diff --git a/src/git.cpp b/src/git.cpp
index 177b24a..afa5234 100644
--- a/src/git.cpp
+++ b/src/git.cpp
@@ -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) {
diff --git a/src/namespace_def.cpp b/src/namespace_def.cpp
index 80c2551..2960c36 100644
--- a/src/namespace_def.cpp
+++ b/src/namespace_def.cpp
@@ -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)

Re: [QGIT PATCH/RFC]

[Marco Costalba]

Hi Abdel,

On Wed, Nov 4, 2009 at 15:56, Abdelrazak Younes <younes@lyx.org> wrote:
> 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...
>

Unfortunatly I cannot say the same here ;-)


>-class ShaString;
>+typedef QByteArray ShaString;

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

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

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

Re: [QGIT PATCH/RFC]

[Abdelrazak Younes]

Marco Costalba wrote:
> Hi Abdel,
>
> On Wed, Nov 4, 2009 at 15:56, Abdelrazak Younes <younes@lyx.org> wrote:
>   
>> 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...
>>
>>     
>
> Unfortunatly I cannot say the same here ;-)
>
>
>   
>> -class ShaString;
>> +typedef QByteArray ShaString;
>>     
>
> ...... cut ......
>
>   
>>  uint qHash(const ShaString& s) { // fast path, called 6-7 times per
>> revision
>>
>>     
>
> Function:
>
> uint qHash(const QByteArray&);
>
> is already defined in the Qt Core libraries, so I have a link error
> with your patch.
>   

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 ;-)

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

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.

Re: [QGIT PATCH/RFC]

[Abdelrazak Younes]

Marco Costalba wrote:
>
>>  uint qHash(const ShaString& s) { // fast path, called 6-7 times per
>> revision
>>
>>     
>
> Function:
>
> uint qHash(const QByteArray&);
>
> is already defined in the Qt Core libraries, so I have a link error
> with your patch.
>   

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

Abdel.

Re: [QGIT PATCH/RFC]

[Abdelrazak Younes]

Abdelrazak Younes wrote:
> Marco Costalba wrote:
>>
>>>  uint qHash(const ShaString& s) { // fast path, called 6-7 times per
>>> revision
>>>
>>>     
>>
>> Function:
>>
>> uint qHash(const QByteArray&);
>>
>> is already defined in the Qt Core libraries, so I have a link error
>> with your patch.
>>   
>
> By the way, this function of yours is not used anywhere AFAICS.

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.

Re: [QGIT PATCH/RFC]

[Abdelrazak Younes]

Abdelrazak Younes wrote:
>  Abdelrazak Younes wrote:
> > Marco Costalba wrote:
> >>
> >>> uint qHash(const ShaString& s) { // fast path, called 6-7 times
> >>> per revision
> >>>
> >>>
> >>
> >> Function:
> >>
> >> uint qHash(const QByteArray&);
> >>
> >> is already defined in the Qt Core libraries, so I have a link
> >> error with your patch.
> >>
> >
> > By the way, this function of yours is not used anywhere AFAICS.
>
>  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...

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.

Re: [QGIT PATCH/RFC]

[Marco Costalba]

On Thu, Nov 5, 2009 at 11:37, Abdelrazak Younes <younes@lyx.org> wrote:
>
> I recompiled qgit with the Qt version and I didn't notice any performance
> problem with a big repo (Qt).
>

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.


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

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

Re: [QGIT PATCH/RFC]

[Marco Costalba]

On Thu, Nov 5, 2009 at 21:25, Marco Costalba <mcostalba@gmail.com> wrote:
>
> 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.
>

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 :-)

Re: [QGIT PATCH/RFC]

[Abdelrazak Younes]

Marco Costalba wrote:
> On Thu, Nov 5, 2009 at 21:25, Marco Costalba <mcostalba@gmail.com> wrote:
>   
>> 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.
>>
>>     
>
> 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.

Re: [QGIT PATCH/RFC]

[Abdelrazak Younes]

Marco Costalba wrote:
> On Thu, Nov 5, 2009 at 11:37, Abdelrazak Younes <younes@lyx.org> wrote:
>   
>> I recompiled qgit with the Qt version and I didn't notice any performance
>> problem with a big repo (Qt).
>>
>>     
>
> In git we don't need to compute hashes of sha strings because they are
> already hashed !
>   

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

Thanks,
Abdel.