You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/08/28 10:33:36 UTC
[PATCH] [merge-tracking] use prepared statements economically
Hi All,
Find the attached patch.
With regards
Kamesh Jayachandran
[[[
Patch by: Kamesh Jayachandran <ka...@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'.
]]]
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Berlin 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).
>
> Yes, my only point is that it's not really going to be more efficient,
> so we shouldn't commit it if people feel it makes the code less
> readable.
> Particularly if we are going to change the mergeinfo schema anyway.
Refer my other mail to Malcom about the 'time' output with and without
this patch.
I fail to see why this makes code unreadable. Putting (prepare_statement
+ bind(s) + step + finalize) makes it more readable??
Changing the mergeinfo schema should be a different changeset that this one.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Daniel Berlin <db...@dberlin.org>.
> 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).
Yes, my only point is that it's not really going to be more efficient,
so we shouldn't commit it if people feel it makes the code less
readable.
Particularly if we are going to change the mergeinfo schema anyway.
>
> Also, can sqlite3_reset never return an error? You're not checking
> for one.
It can.
>
> Regards,
> Malcolm
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Daniel Berlin <db...@dberlin.org>.
I committed this
On 8/31/06, Kamesh Jayachandran <ka...@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 <ka...@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 <ka...@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
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Kamesh Jayachandran <ka...@collab.net>.
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 <ka...@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
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Malcolm Rowe <ma...@farside.org.uk>.
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 <ka...@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). 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Kamesh Jayachandran <ka...@collab.net>.
Malcolm Rowe wrote:
> On Thu, Aug 31, 2006 at 12:46:07AM +0530, Kamesh Jayachandran wrote:
>
>> Daniel Berlin wrote:
>>
>>> This is fine (though mildly pointless because without being able to
>>> move the entire prepared statement out of the loop, it's probably not
>>> much of a speedup if at all)
>>>
>>>
>> I don't think so. we save the compilation cost. I believe the prepared
>> statements are meant to be compiled once and the place holder values to
>> be substituted based on the need(most often from the loop).
>>
>> Correct me if I am wrong.
>>
>>
>
> 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>
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
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!
> Also, can sqlite3_reset never return an error? You're not checking
> for one.
>
>
Thanks.
Attached patch takes care of that.
With regards
Kamesh Jayachandran
[[[
Patch by: Kamesh Jayachandran <ka...@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'.
]]]
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Aug 31, 2006 at 12:46:07AM +0530, Kamesh Jayachandran wrote:
> Daniel Berlin wrote:
> >This is fine (though mildly pointless because without being able to
> >move the entire prepared statement out of the loop, it's probably not
> >much of a speedup if at all)
> >
> I don't think so. we save the compilation cost. I believe the prepared
> statements are meant to be compiled once and the place holder values to
> be substituted based on the need(most often from the loop).
>
> Correct me if I am wrong.
>
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).
Also, can sqlite3_reset never return an error? You're not checking
for one.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Berlin wrote:
> This is fine (though mildly pointless because without being able to
> move the entire prepared statement out of the loop, it's probably not
> much of a speedup if at all)
>
I don't think so. we save the compilation cost. I believe the prepared
statements are meant to be compiled once and the place holder values to
be substituted based on the need(most often from the loop).
Correct me if I am wrong.
With regards
Kamesh Jayachandran
> On 8/28/06, Kamesh Jayachandran <ka...@collab.net> wrote:
>> Hi All,
>> Find the attached patch.
>>
>> With regards
>> Kamesh Jayachandran
>>
>> [[[
>> Patch by: Kamesh Jayachandran <ka...@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'.
>>
>> ]]]
>>
>>
>>
>> Index: subversion/libsvn_fs_fs/fs_fs.c
>> ===================================================================
>> --- subversion/libsvn_fs_fs/fs_fs.c (revision 21288)
>> +++ 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);
>> +
>> + sqlite3_reset(stmt);
>> }
>> + 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
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] use prepared statements economically
Posted by Daniel Berlin <db...@dberlin.org>.
This is fine (though mildly pointless because without being able to
move the entire prepared statement out of the loop, it's probably not
much of a speedup if at all)
On 8/28/06, Kamesh Jayachandran <ka...@collab.net> wrote:
> Hi All,
> Find the attached patch.
>
> With regards
> Kamesh Jayachandran
>
> [[[
> Patch by: Kamesh Jayachandran <ka...@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'.
>
> ]]]
>
>
>
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 21288)
> +++ 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);
> +
> + sqlite3_reset(stmt);
> }
> + 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
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org