threads / rfc / 21483

RFC patch[QGIT PATCH/RFC]

Subject: [QGIT PATCH/RFC]

## tl;dr

10 messages between Nov 4, 2009 and Nov 6, 2009. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Abdelrazak Younes· Nov 4, 2009, 14:56 UTC · lore
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.

Show changes to 4 files +6 −17

src/cache.cpp, src/common.h, src/git.cpp, src/namespace_def.cpp

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)
Marco Costalba· Nov 5, 2009, 09:41 UTC · re: Abdelrazak Younes · lore

Re: [QGIT PATCH/RFC]

Hi Abdel,
On Wed, Nov 4, 2009 at 15:56, Abdelrazak Younes <younes@lyx.org> wrote:
Show 6 quoted lines
> 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
Abdelrazak Younes· Nov 5, 2009, 09:50 UTC · re: Marco Costalba · lore

Re: [QGIT PATCH/RFC]

Marco Costalba wrote:
Show 35 quoted lines
> 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.
Abdelrazak Younes· Nov 5, 2009, 10:13 UTC · re: Marco Costalba · lore

Re: [QGIT PATCH/RFC]

Marco Costalba wrote:
Show 13 quoted lines
>
>>  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.
Abdelrazak Younes· Nov 5, 2009, 10:19 UTC · re: Abdelrazak Younes · lore

Re: [QGIT PATCH/RFC]

Abdelrazak Younes wrote:
Show 16 quoted lines
> 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.
Abdelrazak Younes· Nov 5, 2009, 10:37 UTC · re: Abdelrazak Younes · lore

Re: [QGIT PATCH/RFC]

Abdelrazak Younes wrote:
Show 23 quoted lines
>  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.
Marco Costalba· Nov 5, 2009, 20:25 UTC · re: Abdelrazak Younes · lore

Re: [QGIT PATCH/RFC]

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

Marco Costalba· Nov 5, 2009, 20:27 UTC · re: Marco Costalba · lore

Re: [QGIT PATCH/RFC]

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

Abdelrazak Younes· Nov 6, 2009, 08:15 UTC · re: Marco Costalba · lore

Re: [QGIT PATCH/RFC]

Marco Costalba wrote:
Show 12 quoted lines
> 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.
Abdelrazak Younes· Nov 6, 2009, 08:16 UTC · re: Marco Costalba · lore

Re: [QGIT PATCH/RFC]

Marco Costalba wrote:
Show 10 quoted lines
> 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.

← back to recent threads