I committed this
On 8/31/06, Kamesh Jayachandran <kamesh@collab.net> wrote:
> Malcolm Rowe wrote:
> > On Thu, Aug 31, 2006 at 02:57:17PM +0530, Kamesh Jayachandran wrote:
> >
> >>> That's the way that prepare/execute works in general.
> >>>
> >>> Whether it actually has any effect in this case, you'd need to confirm
> >>> by testing (and as I think that Dan is saying, it's unlikely to make
> >>> any really significant difference in this case, at which point you
> >>> probably want to start balancing code readability against marginal
> >>> efficiency gains).
> >>>
> >>>
> >>>
> >> Snip from http://www.linuxjournal.com/article/7803
> >> <snip>
> >> The question mark represents a wildcard, a place where real data is
> >> bound before the query actually runs. The query is prepared once, and
> >> for each execution, actual data is bound to the wildcards. This
> >> eliminates the need for careful, paranoid parsing of user-supplied data
> >> for killer characters and potential exploits.
> >> </snip>
> >>
> >>
> >
> > Yes, and? I do understand how prepare/execute works.
> >
> >
> >> I already have 20 merges on /trunk from 20 different branches.
> >> When I did my 21'st merge+commit(21 insertions), I observed the following
> >>
> >> with patch
> >> ------------------
> >> real 0m0.506s
> >> user 0m0.049s
> >> sys 0m0.022s
> >>
> >> without patch
> >> ------------------
> >> real 0m1.181s
> >> user 0m0.038s
> >> sys 0m0.027s
> >>
> >>
> >
> > Thank you. _That_ is the information you should have included with
> > your first patch submission.
> >
> > So, assuming that's a realistic use case, it appears that there's a
> > genuine performance reason to split apart the prepare and execute steps.
> > Dan, is that a fair comment?
> >
> >
> >> I fail to see the point it breaks the code readability, the user at this
> >> code should see only 'mergedrevstart' and 'mergedrevend' are variants
> >> across iterations rest are constants across iterations.
> >> I could not understand how putting (statement creation, all binds, step,
> >> finalize together in the loop) will improve the readability!
> >>
> >>
> >
> > Keeping the code to build, bind and execute the query together means that
> > it's slightly easier to follow what's happening, even if it's slightly
> > less efficient. Code readability and maintainability is far, far more
> > important than micro-managed performance, in almost all cases.
> >
> >
> >> [[[
> >> Patch by: Kamesh Jayachandran <kamesh@collab.net>
> >>
> >> Use the same prepared statement across multiple queries of same kind.
> >> Bind the invariant values outside the 'for loop'.
> >>
> >> * subversion/libsvn_fs_fs/fs_fs.c
> >> (index_path_merge_info):
> >> Create the statement outside the for loop.
> >> Reset the statement after each execution as mandated by sqlite
> >> using sqlite3_reset(Note: reset preserves the bound value, it just
> >> resets the state of the statement for reexecution.)
> >> Bind 'mergedfrom', 'mergedto', 'revision' outside the for loop as they
> >> are invariants across iterations.
> >> Finalize the statement outside the 'for loop'.
> >> ]]]
> >>
> >>
> >
> > The patch looks okay to me, on first glance, but the detailed part of
> > log message is _far_ too detailed (and something appears to have eaten
> > all the leading spaces).
> Seems like my email client issue. I will attach the log so that 'leading
> space' won't get stripped.
>
> > Particularly, the 'note' is inappropriate for
> > the log message - if it should be mentioned anywhere (and I don't think
> > it should), it should be in the code. I'm also not sure what 'queries
> > of same kind' means.
> >
> > Regards,
> > Malcolm
> >
> >
> Attached log addresses the above.
>
> Thanks for your review.
>
> With regards
> Kamesh Jayachandran
>
>
> [[[
> Patch by: Kamesh Jayachandran <kamesh@collab.net>
>
> Use the same prepared statement across multiple invocation of same query.
> Bind the invariant values outside the 'for loop'.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (index_path_merge_info):
> Create the statement outside the for loop.
> Reset the statement after each execution as mandated by sqlite
> using sqlite3_reset.
> Bind 'mergedfrom', 'mergedto', 'revision' outside the for loop as they
> are invariants across iterations.
> Finalize the statement outside the 'for loop'.
>
> ]]]
>
>
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 21302)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -4022,20 +4022,19 @@
> if (from && revlist)
> {
> int i;
> + SQLITE_ERR(sqlite3_prepare(ftd->mtd,
> + "INSERT INTO mergeinfo (revision, mergedto, mergedfrom, mergedrevstart, mergedrevend) VALUES (?, ?, ?, ?, ?);",
> + -1, &stmt, NULL), ftd->mtd);
> + SQLITE_ERR(sqlite3_bind_int64(stmt, 1, new_rev), ftd->mtd);
> + SQLITE_ERR(sqlite3_bind_text(stmt, 2, path, -1, SQLITE_TRANSIENT),
> + ftd->mtd);
> + SQLITE_ERR(sqlite3_bind_text(stmt, 3, from, -1, SQLITE_TRANSIENT),
> + ftd->mtd);
> for (i = 0; i < revlist->nelts; i++)
> {
> svn_merge_range_t *range;
>
> range = APR_ARRAY_IDX(revlist, i, svn_merge_range_t *);
> - SQLITE_ERR(sqlite3_prepare(ftd->mtd,
> - "INSERT INTO mergeinfo (revision, mergedto, mergedfrom, mergedrevstart, mergedrevend) VALUES (?, ?, ?, ?, ?);",
> - -1, &stmt, NULL), ftd->mtd);
> - SQLITE_ERR(sqlite3_bind_int64(stmt, 1, new_rev),
> - ftd->mtd);
> - SQLITE_ERR(sqlite3_bind_text(stmt, 2, path, -1, SQLITE_TRANSIENT),
> - ftd->mtd);
> - SQLITE_ERR(sqlite3_bind_text(stmt, 3, from, -1, SQLITE_TRANSIENT),
> - ftd->mtd);
> SQLITE_ERR(sqlite3_bind_int64(stmt, 4, range->start),
> ftd->mtd);
> SQLITE_ERR(sqlite3_bind_int64(stmt, 5, range->end),
> @@ -4043,9 +4042,10 @@
> if (sqlite3_step(stmt) != SQLITE_DONE)
> return svn_error_create(SVN_ERR_FS_SQLITE_ERROR, NULL,
> sqlite3_errmsg(ftd->mtd));
> -
> - SQLITE_ERR(sqlite3_finalize(stmt), ftd->mtd);
> +
> + SQLITE_ERR(sqlite3_reset(stmt), ftd->mtd);
> }
> + SQLITE_ERR(sqlite3_finalize(stmt), ftd->mtd);
> }
> }
> SQLITE_ERR (sqlite3_prepare(ftd->mtd,
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 31 16:24:28 2006