{"thread":{"id":"21483","subject":"[QGIT PATCH/RFC]","startedAt":"2009-11-04T14:56:48Z","lastAt":"2009-11-06T08:16:31Z","messageCount":10,"participants":["Abdelrazak Younes","Marco Costalba"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"126770","messageId":"4AF19630.2070402@lyx.org","threadId":"21483","inReplyTo":null,"subject":"[QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-04T14:56:48Z","receivedAt":"2009-11-04T14:56:48Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Hello Marco,\n\nWhile recompiling latest qgit4, I stumbled accross this. I am not quite \nsure you used a QLatin1String instead of a QByteArray but the attached \nseems to work fine...\n\nAnyway, I'll let you decide what do to with it.\n\nThanks for QGit,\nAbdel.\n\n\ndiff --git a/src/cache.cpp b/src/cache.cpp \n \n\nindex af18fbf..2d9f415 100644 \n \n\n--- a/src/cache.cpp \n \n\n+++ b/src/cache.cpp \n \n\n@@ -70,11 +70,11 @@ bool Cache::save(const QString& gitDir, const \nRevFileMap& rf, \n\n const ShaString& sha = it.key(); \n \n\n if ( sha == ZERO_SHA_RAW \n \n\n || sha == CUSTOM_SHA_RAW \n \n\n- || sha.latin1()[0] == 'A') // ALL_MERGE_FILES + rev \nsha \n\n+ || sha.at(0) == 'A') // ALL_MERGE_FILES + rev sha \n \n\n continue; \n \n\n\n v.append(it.value());\n- buf.append(sha.latin1()).append('\\0');\n+ buf.append(sha);\n newSize += 41;\n if (newSize > bufSize) {\n dbs(\"ASSERT in Cache::save, out of allocated \nspace\");\ndiff --git a/src/common.h b/src/common.h\nindex ceb62fb..0d65980 100644\n--- a/src/common.h\n+++ b/src/common.h\n@@ -7,6 +7,7 @@\n #ifndef COMMON_H\n #define COMMON_H\n\n+#include \n #include \n #include \n #include \n@@ -49,7 +50,7 @@ class QDataStream;\n class QProcess;\n class QSplitter;\n class QWidget;\n-class ShaString;\n+typedef QByteArray ShaString;\n\n // type shortcuts\n typedef const QString& SCRef;\n@@ -274,18 +275,6 @@ namespace QGit {\n extern const QString SCRIPT_EXT;\n }\n\n-class ShaString : public QLatin1String {\n-public:\n- inline ShaString() : QLatin1String(NULL) {}\n- inline ShaString(const ShaString& sha) : \nQLatin1String(sha.latin1()) {}\n- inline explicit ShaString(const char* sha) : QLatin1String(sha) {}\n-\n- inline bool operator!=(const ShaString& o) const { return \n!operator==(o); }\n- inline bool operator==(const ShaString& o) const {\n-\n- return (latin1() == o.latin1()) || !qstrcmp(latin1(), \no.latin1());\n- }\n-};\n\n class Rev {\n // prevent implicit C++ compiler defaults\ndiff --git a/src/git.cpp b/src/git.cpp\nindex 177b24a..afa5234 100644\n--- a/src/git.cpp\n+++ b/src/git.cpp\n@@ -725,7 +725,7 @@ const Rev* Git::revLookup(SCRef sha, const \nFileHistory* fh) const {\n const Rev* Git::revLookup(const ShaString& sha, const FileHistory* fh) \nconst {\n\n const RevMap& r = (fh ? fh->revs : revData->revs);\n- return (sha.latin1() ? r.value(sha) : NULL);\n+ return (sha.isEmpty() ? NULL : r.value(sha));\n }\n\n bool Git::run(SCRef runCmd, QString* runOutput, QObject* receiver, \nSCRef buf) {\ndiff --git a/src/namespace_def.cpp b/src/namespace_def.cpp\nindex 80c2551..2960c36 100644\n--- a/src/namespace_def.cpp\n+++ b/src/namespace_def.cpp\n@@ -95,7 +95,7 @@ static inline uint hexVal(const uchar* ch) {\n\n uint qHash(const ShaString& s) { // fast path, called 6-7 times per \nrevision\n\n- const uchar* ch = reinterpret_cast(s.latin1());\n+ const uchar* ch = reinterpret_cast(s.data());\n return (hexVal(ch ) << 24)\n + (hexVal(ch + 2) << 20)\n + (hexVal(ch + 4) << 16)\n"},{"id":"126895","messageId":"e5bfff550911050141t751d45a0r4e340fa0d10af366@mail.gmail.com","threadId":"21483","inReplyTo":"4AF19630.2070402@lyx.org","subject":"Re: [QGIT PATCH/RFC]","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2009-11-05T09:41:27Z","receivedAt":"2009-11-05T09:41:27Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"Hi Abdel,\n\nOn Wed, Nov 4, 2009 at 15:56, Abdelrazak Younes wrote:\n> Hello Marco,\n>\n> While recompiling latest qgit4, I stumbled accross this. I am not quite sure\n> you used a QLatin1String instead of a QByteArray but the attached seems to\n> work fine...\n>\n\nUnfortunatly I cannot say the same here ;-)\n\n\n>-class ShaString;\n>+typedef QByteArray ShaString;\n\n...... cut ......\n\n>\n>  uint qHash(const ShaString& s) { // fast path, called 6-7 times per\n> revision\n>\n\nFunction:\n\nuint qHash(const QByteArray&);\n\nis already defined in the Qt Core libraries, so I have a link error\nwith your patch.\n\nBTW I don't think I have understood the reason of your patch. Do you\nhave a compile error or something ?\n\nThanks\n\nMarco\n"},{"id":"126897","messageId":"4AF29FF2.1010000@lyx.org","threadId":"21483","inReplyTo":"e5bfff550911050141t751d45a0r4e340fa0d10af366@mail.gmail.com","subject":"Re: [QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-05T09:50:42Z","receivedAt":"2009-11-05T09:50:42Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Marco Costalba wrote:\n> Hi Abdel,\n>\n> On Wed, Nov 4, 2009 at 15:56, Abdelrazak Younes wrote:\n> \n>> Hello Marco,\n>>\n>> While recompiling latest qgit4, I stumbled accross this. I am not quite sure\n>> you used a QLatin1String instead of a QByteArray but the attached seems to\n>> work fine...\n>>\n>> \n>\n> Unfortunatly I cannot say the same here ;-)\n>\n>\n> \n>> -class ShaString;\n>> +typedef QByteArray ShaString;\n>> \n>\n> ...... cut ......\n>\n> \n>> uint qHash(const ShaString& s) { // fast path, called 6-7 times per\n>> revision\n>>\n>> \n>\n> Function:\n>\n> uint qHash(const QByteArray&);\n>\n> is already defined in the Qt Core libraries, so I have a link error\n> with your patch.\n> \n\nWeird... it links just fine here... anyway this can be solved by \nrenaming your version. Or just using the Qt version if that does the \nsame thing ;-)\n\n> BTW I don't think I have understood the reason of your patch. Do you\n> have a compile error or something ?\n> \n\nNo, I had some warnings so I looked at the code and I just thought that \nQLatin1String was not appropriate here and overkill. And QByteArray \nshould be faster...\n\nAnyway, this was just FYI, I don't think this patch is important at all :-)\n\nAbdel.\n"},{"id":"126898","messageId":"4AF2A538.7040303@lyx.org","threadId":"21483","inReplyTo":"e5bfff550911050141t751d45a0r4e340fa0d10af366@mail.gmail.com","subject":"Re: [QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-05T10:13:12Z","receivedAt":"2009-11-05T10:13:12Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Marco Costalba wrote:\n>\n>> uint qHash(const ShaString& s) { // fast path, called 6-7 times per\n>> revision\n>>\n>> \n>\n> Function:\n>\n> uint qHash(const QByteArray&);\n>\n> is already defined in the Qt Core libraries, so I have a link error\n> with your patch.\n> \n\nBy the way, this function of yours is not used anywhere AFAICS.\n\nAbdel.\n"},{"id":"126899","messageId":"4AF2A69F.1090802@lyx.org","threadId":"21483","inReplyTo":"4AF2A538.7040303@lyx.org","subject":"Re: [QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-05T10:19:11Z","receivedAt":"2009-11-05T10:19:11Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Abdelrazak Younes wrote:\n> Marco Costalba wrote:\n>>\n>>> uint qHash(const ShaString& s) { // fast path, called 6-7 times per\n>>> revision\n>>>\n>>> \n>>\n>> Function:\n>>\n>> uint qHash(const QByteArray&);\n>>\n>> is already defined in the Qt Core libraries, so I have a link error\n>> with your patch.\n>> \n>\n> By the way, this function of yours is not used anywhere AFAICS.\n\nOk, now I understand that this is used by QHash, sorry.\n\nI have gcc-4.4.1 so maybe that's the reason why linking works in my \ncase. But I don't which version of the qhash() function does take \nprecedence...\n\nAbdel.\n"},{"id":"126901","messageId":"4AF2AAFD.9000309@lyx.org","threadId":"21483","inReplyTo":"4AF2A69F.1090802@lyx.org","subject":"Re: [QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-05T10:37:49Z","receivedAt":"2009-11-05T10:37:49Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Abdelrazak Younes wrote:\n> Abdelrazak Younes wrote:\n> > Marco Costalba wrote:\n> >>\n> >>> uint qHash(const ShaString& s) { // fast path, called 6-7 times\n> >>> per revision\n> >>>\n> >>>\n> >>\n> >> Function:\n> >>\n> >> uint qHash(const QByteArray&);\n> >>\n> >> is already defined in the Qt Core libraries, so I have a link\n> >> error with your patch.\n> >>\n> >\n> > By the way, this function of yours is not used anywhere AFAICS.\n>\n> Ok, now I understand that this is used by QHash, sorry.\n>\n> I have gcc-4.4.1 so maybe that's the reason why linking works in my\n> case. But I don't which version of the qhash() function does take\n> precedence...\n\nFYI I just verified that your version does take precedence over the Qt one.\n\nAnyway, if you like the patch, we could just inherit from QByteArray \ninstead of typedef it so that it links with your system.\n\nOut of curiosity I just had a look at the two versions, yours:\n\n********************************\n// definition of an optimized sha hash function\nstatic inline uint hexVal(const uchar* ch) {\n return (*ch < 64 ? *ch - 48 : *ch - 87);\n}\n\nuint qHash(const ShaString& s) { // fast path, called 6-7 times per revision\n\n const uchar* ch = reinterpret_cast(s.data());\n\n return (hexVal(ch ) << 24)\n + (hexVal(ch + 2) << 20)\n + (hexVal(ch + 4) << 16)\n + (hexVal(ch + 6) << 12)\n + (hexVal(ch + 8) << 8)\n + (hexVal(ch + 10) << 4)\n + hexVal(ch + 12);\n}\n********************************\n\nAnd Qt's version:\n\n********************************\nstatic uint hash(const uchar *p, int n)\n{\n uint h = 0;\n uint g;\n\n while (n--) {\n h = (h << 4) + *p++;\n if ((g = (h & 0xf0000000)) != 0)\n h ^= g >> 23;\n h &= ~g;\n }\n return h;\n}\n\nuint qHash(const QBitArray &bitArray)\n{\n int m = bitArray.d.size() - 1;\n uint result = hash(reinterpret_cast(bitArray.d.constData()), qMax(0, m));\n\n // deal with the last 0 to 7 bits manually, because we can't trust that\n // the padding is initialized to 0 in bitArray.d\n int n = bitArray.size();\n if (n & 0x7)\n result = ((result << 4) + bitArray.d.at(m)) & ((1 << n) - 1);\n return result;\n}\n\n********************************\n\nI recompiled qgit with the Qt version and I didn't notice any \nperformance problem with a big repo (Qt).\n\nJust tell me if this is not interesting to you and I'll shut up :-)\n\nAbdel.\n"},{"id":"126922","messageId":"e5bfff550911051225s13c6e39dh355dc3ab1c0623f@mail.gmail.com","threadId":"21483","inReplyTo":"4AF2AAFD.9000309@lyx.org","subject":"Re: [QGIT PATCH/RFC]","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2009-11-05T20:25:32Z","receivedAt":"2009-11-05T20:25:32Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Thu, Nov 5, 2009 at 11:37, Abdelrazak Younes wrote:\n>\n> I recompiled qgit with the Qt version and I didn't notice any performance\n> problem with a big repo (Qt).\n>\n\nIn git we don't need to compute hashes of sha strings because they are\nalready hashed !\n\nThat's the idea of using a custom hashing function that does nothing\nbut taking the first chars of the sha string. Instead the general\npurpose Qt hashing must do real work because it has to work for any\nstring.\n\nWhen I tested I _found_ a speed difference, but now I don't remember\nof how much. Be sure you have warm cache when doing the test (press\nF5) for few times to be sure all is in RAM.\n\n\n> Just tell me if this is not interesting to you and I'll shut up :-)\n>\n\nNo, it is very interesting indeed. My bad I have no time for net access today.\n\nIf QByteArray is faster then QLatin1String() we should definitely\nchange. But if I don't remember wrong QLatin1String() is already\nimplemented above a QByteArray and the methods that we use are\ninherited directly from a QByteArray, but I may be wrong. I don't have\naccess to the sources now. I only remember that when I implemented\nthat part it took a good amount of time and testing ;-)\n\nThanks\nMarco\n"},{"id":"126923","messageId":"e5bfff550911051227g257312b2idf729b9451d1bb@mail.gmail.com","threadId":"21483","inReplyTo":"e5bfff550911051225s13c6e39dh355dc3ab1c0623f@mail.gmail.com","subject":"Re: [QGIT PATCH/RFC]","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2009-11-05T20:27:32Z","receivedAt":"2009-11-05T20:27:32Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Thu, Nov 5, 2009 at 21:25, Marco Costalba wrote:\n>\n> When I tested I _found_ a speed difference, but now I don't remember\n> of how much. Be sure you have warm cache when doing the test (press\n> F5) for few times to be sure all is in RAM.\n>\n\nSorry, I forgot. Be sure also our custom hashing function is actually\ncalled in your binary and the compiler didn't linked the Qt one\ninstead :-)\n"},{"id":"126959","messageId":"4AF3DB29.6010908@lyx.org","threadId":"21483","inReplyTo":"e5bfff550911051227g257312b2idf729b9451d1bb@mail.gmail.com","subject":"Re: [QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-06T08:15:37Z","receivedAt":"2009-11-06T08:15:37Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Marco Costalba wrote:\n> On Thu, Nov 5, 2009 at 21:25, Marco Costalba wrote:\n> \n>> When I tested I _found_ a speed difference, but now I don't remember\n>> of how much. Be sure you have warm cache when doing the test (press\n>> F5) for few times to be sure all is in RAM.\n>>\n>> \n>\n> Sorry, I forgot. Be sure also our custom hashing function is actually\n> called in your binary and the compiler didn't linked the Qt one\n> instead :-)\n> \n\nYes I made sure of that (see my other mail).\n\nAbdel.\n"},{"id":"126960","messageId":"4AF3DB5F.3030704@lyx.org","threadId":"21483","inReplyTo":"e5bfff550911051225s13c6e39dh355dc3ab1c0623f@mail.gmail.com","subject":"Re: [QGIT PATCH/RFC]","fromName":"Abdelrazak Younes","fromEmail":"younes@lyx.org","sentAt":"2009-11-06T08:16:31Z","receivedAt":"2009-11-06T08:16:31Z","isPatch":true,"sender":{"key":"younes@lyx.org","avatar":null},"body":"Marco Costalba wrote:\n> On Thu, Nov 5, 2009 at 11:37, Abdelrazak Younes wrote:\n> \n>> I recompiled qgit with the Qt version and I didn't notice any performance\n>> problem with a big repo (Qt).\n>>\n>> \n>\n> In git we don't need to compute hashes of sha strings because they are\n> already hashed !\n> \n\nNow you tell it, it's obvious indeed :-)\n\nThanks,\nAbdel.\n"}]}