You should have a look in my library_feature branch :-) I already use QStringbuilder there.
http://bazaar.launchpad.net/~max-linke/mixxx/library_features/view/head:/mixxx/src/library/dao/directorydao.cpp As for the const vs #define discussion I'm for 'static const' like Daniel. On Wed, 1 May 2013 23:45:45 +0200 Daniel Schürmann <[email protected]> wrote: > Cool RJ :-) > > It was always there but I never noticed: > http://qt-project.org/doc/qt-4.8/qstring.html#more-efficient-string-construction > > Let us use this! > > > > > 2013/5/1 RJ Ryan <[email protected]> > > > > > > > > > On Wed, May 1, 2013 at 4:03 PM, Daniel Schürmann > > <[email protected]>wrote: > > > >> Hi, > >> > >> the typo resistance is an other good point for using constants. > >> > >> --- > >> > >> The performance win does not happen when issue a query, it happens > >> when starting Mixxx. > >> > >> const QString LIBRARYTABLE_ID = "id"; > >> > >> "id" is in text segment after starting Mixxx. The QString > >> constructor converts the string to 16 bit unicode and stores it on > >> the heap after malloc. > >> > >> const char* = "id"; > >> > >> is simply a pointer to the text segment. > >> > > > > Ah, yes -- I agree there is a cost (again, tiny) during global > > initialization. I was referring to the cost of constructing the SQL > > query in Steven's example though. > > > > #define PLAYLIST_TABLE "PlaylistTracks" > > > > QString sqlQuery = QString("SELECT FROM " PLAYLIST_TABLE " ..."); > > > > This is 1 malloc and no concatenation. The compiler joins the > > multiple string literals together into one string literal. > > > > Whereas > > > > const QString PLAYLIST_TABLE = "PlaylistTracks"; > > > > QString sqlQuery = QString("SELECT FROM " + PLAYLIST_TABLE + "..."); > > > > Involves potentially 2 mallocs before the outer QString constructor > > is called (the outer QString will probably use Qt's implicit > > sharing so no malloc will happen). > > > > Since often in the DAOs very long queries are constructed by > > strings of + operators and QString string interpolation, there is a > > whole lot of malloc and copy going on every time we want to format > > a query. > > > > I still say it's negligible compared to any real work going on -- > > but relative to global static initialization (a one time cost at > > bootup on the order of tens of microseconds, max) I would say this > > is much more expensive since it happens every time we construct a > > query and often does a lot more work than a single malloc / memcpy. > > > > I read a post a while back about QStringBuilder. In Qt 4.6 you can > > basically define QT_USE_QSTRINGBUILDER and it converts QString's > > operator + to use lazy evaluation. We may as well turn that on? > > > > http://blog.qt.digia.com/blog/2011/06/13/string-concatenation-with-qstringbuilder/ > > > > > > > >> > >> I am also not sure if putting this to trackdao.h will duplicate the > >> sting in text segment each time trackdao.h is included. So we > >> should make them static members of TrackDAO to be sure. > >> > > > > The QString definitions will appear in the data section of every .o > > that includes trackdao.h. Ideally the linker would collapse the > > duplicates but I'm guessing it doesn't :). Someone could easily > > check by running 'strings' on the mixxx binary. > > > > > >> Just two cents from an embedded developer ;-) > >> > >> Kind regards, > >> > >> Daniel > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> 2013/5/1 RJ Ryan <[email protected]> > >> > >>> The main reason for me is that they are a protection measure > >>> against making typos in SQL queries. You'd think a typo would get > >>> noticed but not all queries fail immediately when they have a > >>> mistake in them. Some of them just return no results, or return > >>> wrong results. For queries that are off the beaten path, we end > >>> up with issues like > >>> this<https://bugs.launchpad.net/mixxx/+bug/1175024>(which Steven > >>> just fixed). > >>> > >>> In fact, I would prefer we totally got rid of writing SQL queries > >>> directly in code and instead composed them using declarative C++ > >>> classes which are then compiled down to SQL. Not quite a full-on > >>> ORM, just something for composing SQL queries (like SQLAlchemy > >>> for C++). > >>> > >>> > >>> On Wed, May 1, 2013 at 2:56 PM, Owen Williams > >>> <[email protected]>wrote: > >>> > >>>> Are the constants really a good idea? What is the purpose > >>>> behind using them? Ideally the schema shouldn't be changing > >>>> very often so having them hardcoded seems ok from a maintenance > >>>> perspective. And from a readability perspective, the constant > >>>> method looks really ugly to me. I'm just wondering what the > >>>> other side of the argument is. > >>>> > >>>> owen > >>>> > >>>> On Wed, 2013-05-01 at 14:42 -0400, RJ Ryan wrote: > >>>> > On Wed, May 1, 2013 at 2:15 PM, Daniel Schürmann > >>>> > <[email protected]> wrote: > >>>> > Hi Steven, > >>>> > > >>>> > Yes, we should solve this issue and use the const > >>>> > values in any case. > >>>> > This allows to rely on IDE features like "display call > >>>> > tree". > >>>> > > >>>> > > >>>> > > >>>> > I think we should stick with whatever makes the code easiest to > >>>> > maintain (which would be using the constants when possible) -- > >>>> > maybe the constants should be renamed to be a little easier on > >>>> > the eyes? A lot of the DAO code doesn't use the constants, > >>>> > that's true. We should fix those queries. > >>>> > > >>>> > > >>>> > But I am not sure if the use of const QString is > >>>> > performance neutral to const char* and #define because the > >>>> > heavy QSting constructor is called. > >>>> > > >>>> > So IMHO we should change it to const char*. > >>>> > > >>>> > > >>>> > There are a lot of non-performance-neutral choices we make in > >>>> > Mixxx, but that alone isn't enough to make a decision, we have > >>>> > to take the context into account. Compared to the > >>>> > QSqlQuery::exec() that comes immediately after all these > >>>> > queries, some string concatenation isn't going to break the > >>>> > bank, cycle-wise. Processors are pretty good at memcpy these > >>>> > days :). > >>>> > > >>>> > > >>>> > Kind regards, > >>>> > > >>>> > Daniel > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > Am 01.05.2013 17:32, schrieb Steven Boswell II: > >>>> > > >>>> > > Most of the DAO header files in src/library/dao have > >>>> > > definitions for the names of tables, and the names of > >>>> > > columns in the tables, but they're used > >>>> > > sporadically; quite often, references to them are > >>>> > > hardcoded. > >>>> > > > >>>> > > > >>>> > > I'm writing a new feature (the aforementioned > >>>> > > auto-DJ-crates), and the necessary info is stored in > >>>> > > a temporary table, so I'm writing several SQL > >>>> > > queries. I'm just wondering if I should hardcode > >>>> > > the table/column references (which won't > >>>> > > automatically get updated if the underlying names > >>>> > > change), or try to write them using the table/column > >>>> > > names defined in header files (which makes the query > >>>> > > harder to read). > >>>> > > > >>>> > > > >>>> > > Here's an example. The comment is the hardcoded, > >>>> > > human-readable version, and following it is the > >>>> > > hard-to-read one that'll adapt automatically to > >>>> > > changes. > >>>> > > > >>>> > > > >>>> > > > >>>> > > // INSERT INTO temp_autodj_crates (track_id, > >>>> > > craterefs, timesplayed, autodjrefs) SELECT > >>>> > > crate_tracks.track_id, COUNT (*), > >>>> > > library.timesplayed, 0 FROM crate_tracks, library > >>>> > > WHERE crate_tracks.crate_id IN (SELECT id FROM > >>>> > > crates WHERE autodj = 1) AND crate_tracks.track_id = > >>>> > > library.id AND library.mixxx_deleted = 0 GROUP BY > >>>> > > crate_tracks.track_id, library.timesplayed; strQuery > >>>> > > = QString ("INSERT INTO " AUTODJCRATES_TABLE " (" > >>>> > > AUTODJCRATESTABLE_TRACKID ", " > >>>> > > AUTODJCRATESTABLE_CRATEREFS ", " > >>>> > > AUTODJCRATESTABLE_TIMESPLAYED ", " > >>>> > > AUTODJCRATESTABLE_AUTODJREFS ")" " SELECT " > >>>> > > CRATE_TRACKS_TABLE ".%1 , COUNT (*), " LIBRARY_TABLE > >>>> > > ".%2, 0 FROM " CRATE_TRACKS_TABLE ", " LIBRARY_TABLE > >>>> > > " WHERE " CRATE_TRACKS_TABLE ".%4 IN " > >>>> > > "(SELECT %5 FROM " CRATE_TABLE " WHERE %6 = > >>>> > > 1) AND " CRATE_TRACKS_TABLE ".%1 = " LIBRARY_TABLE > >>>> > > ".%7 AND " LIBRARY_TABLE > >>>> > > ".%3 == 0 GROUP BY " CRATE_TRACKS_TABLE > >>>> > > ".%1, " LIBRARY_TABLE ".%2") > >>>> > > .arg > >>>> > > (CRATETRACKSTABLE_TRACKID) // %1 .arg > >>>> > > (LIBRARYTABLE_TIMESPLAYED) // %2 .arg > >>>> > > (LIBRARYTABLE_MIXXXDELETED) // %3 .arg > >>>> > > (CRATETRACKSTABLE_CRATEID) // %4 .arg > >>>> > > (CRATETABLE_ID) // %5 .arg > >>>> > > (CRATETABLE_AUTODJ) // %6 .arg > >>>> > > (LIBRARYTABLE_ID); // %7 > >>>> > > > >>>> > > > >>>> > > > >>>> > > Which should I do? > >>>> > > > >>>> > > > >>>> > > Pretty soon, I'll make a blueprint for the > >>>> > > auto-DJ-crates feature. > >>>> > > > >>>> > > > >>>> > > > >>>> > > Steven Boswell > >>>> > > > >>>> > > > >>>> > > > >>>> > > > >>>> > > > >>>> ------------------------------------------------------------------------------ > >>>> > > Introducing AppDynamics Lite, a free troubleshooting > >>>> > > tool > >>>> for Java/.NET > >>>> > > Get 100% visibility into your production application > >>>> > > - at > >>>> no cost. > >>>> > > Code-level diagnostics for performance bottlenecks > >>>> > > with <2% > >>>> overhead > >>>> > > Download for free and get started troubleshooting in > >>>> minutes. > >>>> > > http://p.sf.net/sfu/appdyn_d2d_ap1 > >>>> > > > >>>> > > > >>>> > > _______________________________________________ > >>>> > > Get Mixxx, the #1 Free MP3 DJ Mixing software Today > >>>> > > http://mixxx.org > >>>> > > > >>>> > > > >>>> > > Mixxx-devel mailing list > >>>> > > [email protected] > >>>> > > https://lists.sourceforge.net/lists/listinfo/mixxx-devel > >>>> > > >>>> > > >>>> > > >>>> > > >>>> ------------------------------------------------------------------------------ > >>>> > Introducing AppDynamics Lite, a free troubleshooting > >>>> > tool for Java/.NET > >>>> > Get 100% visibility into your production application - > >>>> > at no cost. > >>>> > Code-level diagnostics for performance bottlenecks > >>>> > with <2% overhead > >>>> > Download for free and get started troubleshooting in > >>>> > minutes. http://p.sf.net/sfu/appdyn_d2d_ap1 > >>>> > _______________________________________________ > >>>> > Get Mixxx, the #1 Free MP3 DJ Mixing software Today > >>>> > http://mixxx.org > >>>> > > >>>> > > >>>> > Mixxx-devel mailing list > >>>> > [email protected] > >>>> > https://lists.sourceforge.net/lists/listinfo/mixxx-devel > >>>> > > >>>> > > >>>> > > >>>> ------------------------------------------------------------------------------ > >>>> > Introducing AppDynamics Lite, a free troubleshooting tool for > >>>> Java/.NET > >>>> > Get 100% visibility into your production application - at no > >>>> > cost. Code-level diagnostics for performance bottlenecks with > >>>> > <2% overhead Download for free and get started troubleshooting > >>>> > in minutes. http://p.sf.net/sfu/appdyn_d2d_ap1 > >>>> > _______________________________________________ Get Mixxx, the > >>>> > #1 > >>>> Free MP3 DJ Mixing software Today http://mixxx.org Mixxx-devel > >>>> mailing list [email protected] > >>>> https://lists.sourceforge.net/lists/listinfo/mixxx-devel > >>>> > >>>> > >>>> > >>> > >> > > ------------------------------------------------------------------------------ Introducing AppDynamics Lite, a free troubleshooting tool for Java/.NET Get 100% visibility into your production application - at no cost. Code-level diagnostics for performance bottlenecks with <2% overhead Download for free and get started troubleshooting in minutes. http://p.sf.net/sfu/appdyn_d2d_ap1 _______________________________________________ Get Mixxx, the #1 Free MP3 DJ Mixing software Today http://mixxx.org Mixxx-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mixxx-devel
