> On Сен. 25, 2015, 3:12 д.п., Milian Wolff wrote: > > src/codecs/doctermscodec.cpp, line 73 > > <https://git.reviewboard.kde.org/r/125362/diff/1/?file=405124#file405124line73> > > > > this and the below cases look fine to me - anything else sounds super > > dangerous and could leak to subtle danlging pointers when the raw data > > escapes out. > > Milian Wolff wrote: > to make it clear: I'm with Igor here. > > Vishes, if you really want this, and can proof it's safe, I highly > suggest you introduce another wrapper instead of (re)using QByteArray to make > your intent clear. Also, have you thought of using std::string instead, which > is small-string optimized and thus can probably safe a ton of allocation for > the usual case of strings with less than 24 chars (or something like that)?
One possible solution is to keep track of [MDB_cursor](http://symas.com/mdb/doc/group__internal.html#structMDB__cursor), seems like they keep track of the data when data moves. I'm still not sure if this optimization makes sense. It happends only when indexing, which is itself pretty damn slow with LMDB (and supposed to be so), so saving few MB of memory by not copying keys (not even persistently! just when indexing) might be the case of premature optimization. At least in my case it didn't harm noticeably at all. Yeah, I guess there are several places where it can stay, but I decided to remove it just to make code clearer (so bugs like that won't pop up later...). - Igor ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125362/#review85910 ----------------------------------------------------------- On Сен. 23, 2015, 2:42 п.п., Igor Poboiko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125362/ > ----------------------------------------------------------- > > (Updated Сен. 23, 2015, 2:42 п.п.) > > > Review request for Baloo. > > > Repository: baloo > > > Description > ------- > > I've noted following bug: sometypes e.g "PDF" (T5) files had a "Folder" (T9) > type in KRunner. > "balooshow -x" showed that it has a "T5", while "baloosearch --type folder" > still listed it. > > * Debugging showed that it appears somewhere in WritingTransaction::commit() > * There wasn't any WritingTransaction::m_pendingOperations["T9"] access at > all > * This hash contained "T9" key (QHash::keys().contains("T9") == true), but > it didn't (QHash::contains("T9") == false and QHash::count("T9") == 0) > * Because of that QHashIterator fails miserably iterating over non-existing > values (e.g iter.value() returns some value with some data for that > non-existing values) > * Bisection showed that QHash got corrupted at "documentTermsDB.put(id, > docTerms)" (engine/writingtransaction.cpp:185), to be specific - on mdb_put() > line > > That was the bug itself. The problem is that QByteArray::fromRawData() is > used everywhere, which does not copy data from DB but just stores a pointer > to some place in memory-mapped file in DB. And it doesn't know if data where > it points to changed, leading to undefined behavior like that. > > This patch removes ::fromRawData() calls replacing it by copy-constructors. > (maybe somewhere we can leave it, but I'm not sure it's a optimization we > should care about) > > > Diffs > ----- > > src/codecs/doctermscodec.cpp e8801f9 > src/codecs/postingcodec.cpp 1edb645 > src/engine/documentdatadb.cpp 690df70 > src/engine/documentdb.cpp ea0cb66 > src/engine/idfilenamedb.cpp d4e1eb1 > src/engine/positiondb.cpp 568dc54 > src/engine/postingdb.cpp e183db5 > > Diff: https://git.reviewboard.kde.org/r/125362/diff/ > > > Testing > ------- > > After applying this patch I have no more files in wrong category, so the > issue is gone. > > > Thanks, > > Igor Poboiko > >
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
