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 13:52:04 2006