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
