You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by "Knut Anders Hatlen (JIRA)" <ji...@apache.org> on 2007/03/02 10:44:50 UTC
[jira] Commented: (DERBY-827) Performance can be improved by
re-using language ResultSets across Activation executions.
[ https://issues.apache.org/jira/browse/DERBY-827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12477237 ]
Knut Anders Hatlen commented on DERBY-827:
------------------------------------------
Thanks for writing the test, Dyre. I think it is a valuable addition
to the test suite. I committed rsfromps.v1 with revision 513679.
Some small comments:
- assertRow() and assertResultSet() duplicate functionality that is
already implemented in JDBC.assertRowInResultSet() and
JDBC.assertFullResultSet(). Perhaps they can be reused?
- the use of try/finally in assertResultSet() might hide the
original error if an exception is thrown from within the finally
block.
- the suite() method only runs the test in embedded mode. I think
this is OK since it's a test of language result sets, but it would
be good if there were a comment stating that explicitly. Or the
test could be enabled in network client mode too (could be
achieved by creating two test suites where one of them is wrapped
by TestConfiguration.clientServerDecorator()).
- perhaps the empty test methods (those flagged with TODO) should be
commented out? Now JUnit will report that they ran successfully
whereas they haven't actually tested anything.
> Performance can be improved by re-using language ResultSets across Activation executions.
> -----------------------------------------------------------------------------------------
>
> Key: DERBY-827
> URL: https://issues.apache.org/jira/browse/DERBY-827
> Project: Derby
> Issue Type: Improvement
> Components: Performance
> Reporter: Daniel John Debrunner
> Attachments: derby827_draft_reuse_result_sets.txt, derby827_update920.txt, rsfromps.v1.diff, rsfromps.v1.stat, rsfromps_prelim.diff, rsfromps_prelim2.diff
>
>
> >Shouldn't DistinctScalarAggregateRS implement a close or a finish method
> >>(not sure what the difference is) and close the scan controller there.
> The close() and finish() methods are actually explained in their javadoc
> in the language org.apache.derby.iapi.sql.ResultSet class.
> [note this is not a JDBC java.sql.ResultSet object]
> close() - Tells the system that there will be no more calls to
> getNextRow() (until the next open() call)
> finish() - Tells the system that there will be no more access to any
> database information via this result set
> So close means the ResultSet may be opened again for more access, while
> finish means it will not be used again.
> However, their use in the code always doesn't match that, and that does
> cause confusion, at least to me.
> Language ResultSets (not JDBC ones) can be and are opened multiple
> times, for example when scanning a table multiple times within a join.
> An Activation, which represents the internal state of
> java.sql.PreparedStatement object & has the lifetime of the
> java.sql.PreparedStatement, contains a top-level language ResultSet.
> This top-level language ResultSet provides the execution of the SQL
> statement, DML, DDL or a query. The top-level ResultSet may contain
> other ResultSets and could be seen as a tree structure. For the simple
> case of a primary key lookup query like:
> select name from customer where id = ?
> The activation would contain this:
> top result set
> ProjectRestrictRS << IndexRowToBaseRowRS << TableScanRS
> Now for some reason, even though the api of ResultSet say they can be
> re-used, and in some cases they are, this result set tree is thrown away
> after each execution. That is, the top result set has its finish()
> method called and then the activation removes its reference to it. Then
> on the next execution a new (identical) tree is set up.
> There is potential for a huge performance gain if this top level result
> set and its tree are re-used and have the same lifetime as the
> Activation. The saving comes in two forms, not having to create many
> objects on each execution, and not creating short-lived objects for the
> garbage collector to handle.
> I made a simple fix, it's a couple of lines of code, just calling close
> & finish at the correct times, and for the above simple primary key
> lookup query, the performance went from 17,300 to 24,000 selects per
> second (cached data, single user). I'll post a patch shortly as an
> indication of the direction, once I can separate it from other changes
> in my client.
> However, I'm running the Derby tests and there are some (maybe 25-30)
> failures, I think because not all the language ResultSet implementations
> are correctly written to be re-opened. Interestingly, the first failure
> I saw was in an aggregrate test, which goes back to the issue Manish saw.
> Even if derbyall passed I would be nervous about submitting this patch
> for real, because I don't think there's a lot of testing using repeat
> executions of PreparedStatements in the tests. The ij tests mainly use
> Statement, this is a single use of an activation so this change would
> not affect them. Thus such a patch could regress Derby by making it more
> likely existing bugs would be exposed.
> Given the performance gains, I think we need to start re-using
> ResultSets from Activation, and devise a way to ensure the testing
> covers the re-use. The main issue is there is a large number of
> ResultSet implementations to cover.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: JUnit common methods & assert messages WAS Re: [jira] Commented:
(DERBY-827) Performance can be improved by re-using language ResultSets across
Activation executions.
Posted by "Jean T. Anderson" <jt...@bristowhill.com>.
thanks for the great explanation.
-jean
Daniel John Debrunner wrote:
> Jean T. Anderson wrote:
>
>> Daniel John Debrunner wrote:
>> ...
>>
>>> This style of code
>>> can be expensive at runtime:
>>> for (int i = 0; i < count; i++) {
>>> assertEquals("Testing at iteration " + i, f(i), g(i));
>>> }
>>
>> I'm a fresh guinea pig eager for advice ....
>
> The issue with the example above is the message
> "Testing at iteration " + i
>
> This compiles down to code something like:
>
> StringBuffer sb = new StringBuffer("Testing at iteration ");
> sb.append(i);
> sb.toString();
>
> That's a minimum creation of three objects (StringBuffer, char[] and
> String) and at worse five (two more char[]).
>
> Thus some executed code and several object creations for a string object
> that is never used (because tests don't fail in the common case).
>
>> In CallableTest.java I have this:
>>
>> for(int i=0; i< updateCount.length; i++){
>> if(i == 1) // The second command in the batch failed.
>> assertEquals("Batch updateCount", -3, updateCount[i]);
>> else
>> assertEquals("Batch updateCount", -1, updateCount[i]);
>> }
>
> In this example the string message is fixed, so there's no problem with
> the overhead. Assuming the message was something like:
>
> "Batch updateCount at " + i
>
> then the following does make sense. Though I'm not sure it's better, it
> seems to me to be less clear. I'm not sure of a good solution here.
> Maybe performance of the tests is not critical, but then again the
> faster the tests run the better it is for everyone. One issue is that
> how critical is it to have a variable message? If such a failure is
> expected to be very rare (as we hope) then maybe the best solution is to
> have who ever looks at the test add extra debugging information when
> they are investigating a failure.
>
>> would this be advised?
>>
>> for(int i=0; i< updateCount.length; i++){
>>
>> if( (i == 1) && (updateCount[i] != -3) )
>> assertEquals("Batch updateCount", -3, updateCount[i]);
>> else if (updateCount[i] != -1)
>> assertEquals("Batch updateCount", -1, updateCount[i]);
>> }
>
>
> Dan.
Re: JUnit common methods & assert messages WAS Re: [jira] Commented:
(DERBY-827) Performance can be improved by re-using language ResultSets across
Activation executions.
Posted by Daniel John Debrunner <dj...@apache.org>.
Jean T. Anderson wrote:
> Daniel John Debrunner wrote:
> ...
>> This style of code
>> can be expensive at runtime:
>> for (int i = 0; i < count; i++) {
>> assertEquals("Testing at iteration " + i, f(i), g(i));
>> }
>
> I'm a fresh guinea pig eager for advice ....
The issue with the example above is the message
"Testing at iteration " + i
This compiles down to code something like:
StringBuffer sb = new StringBuffer("Testing at iteration ");
sb.append(i);
sb.toString();
That's a minimum creation of three objects (StringBuffer, char[] and
String) and at worse five (two more char[]).
Thus some executed code and several object creations for a string object
that is never used (because tests don't fail in the common case).
>
> In CallableTest.java I have this:
>
> for(int i=0; i< updateCount.length; i++){
> if(i == 1) // The second command in the batch failed.
> assertEquals("Batch updateCount", -3, updateCount[i]);
> else
> assertEquals("Batch updateCount", -1, updateCount[i]);
> }
In this example the string message is fixed, so there's no problem with
the overhead. Assuming the message was something like:
"Batch updateCount at " + i
then the following does make sense. Though I'm not sure it's better, it
seems to me to be less clear. I'm not sure of a good solution here.
Maybe performance of the tests is not critical, but then again the
faster the tests run the better it is for everyone. One issue is that
how critical is it to have a variable message? If such a failure is
expected to be very rare (as we hope) then maybe the best solution is to
have who ever looks at the test add extra debugging information when
they are investigating a failure.
> would this be advised?
>
> for(int i=0; i< updateCount.length; i++){
>
> if( (i == 1) && (updateCount[i] != -3) )
> assertEquals("Batch updateCount", -3, updateCount[i]);
> else if (updateCount[i] != -1)
> assertEquals("Batch updateCount", -1, updateCount[i]);
> }
Dan.
Re: JUnit common methods & assert messages WAS Re: [jira] Commented:
(DERBY-827) Performance can be improved by re-using language ResultSets across
Activation executions.
Posted by "Jean T. Anderson" <jt...@bristowhill.com>.
Daniel John Debrunner wrote:
...
> This style of code
> can be expensive at runtime:
> for (int i = 0; i < count; i++) {
> assertEquals("Testing at iteration " + i, f(i), g(i));
> }
I'm a fresh guinea pig eager for advice ....
In CallableTest.java I have this:
for(int i=0; i< updateCount.length; i++){
if(i == 1) // The second command in the batch failed.
assertEquals("Batch updateCount", -3, updateCount[i]);
else
assertEquals("Batch updateCount", -1, updateCount[i]);
}
would this be advised?
for(int i=0; i< updateCount.length; i++){
if( (i == 1) && (updateCount[i] != -3) )
assertEquals("Batch updateCount", -3, updateCount[i]);
else if (updateCount[i] != -1)
assertEquals("Batch updateCount", -1, updateCount[i]);
}
thanks,
-jean
JUnit common methods & assert messages WAS Re: [jira] Commented:
(DERBY-827) Performance can be improved by re-using language ResultSets across
Activation executions.
Posted by Daniel John Debrunner <dj...@apache.org>.
Dyre.Tjeldvoll@Sun.COM wrote:
> "Knut Anders Hatlen (JIRA)" <ji...@apache.org> writes:
>
>> [ https://issues.apache.org/jira/browse/DERBY-827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12477237 ]
>>
>> Knut Anders Hatlen commented on DERBY-827:
>> ------------------------------------------
>>
>> Thanks for writing the test, Dyre. I think it is a valuable addition
>> to the test suite. I committed rsfromps.v1 with revision 513679.
>>
>> Some small comments:
>>
>> - assertRow() and assertResultSet() duplicate functionality that is
>> already implemented in JDBC.assertRowInResultSet() and
>> JDBC.assertFullResultSet(). Perhaps they can be reused?
>
> A couple of things about this. When I started working on this test I
> started by copying lang.GroupByExpressionTest since that seemed like a
> pretty standard lang JUnit test. I observed that it had its own
> methods for verifying that result sets. I found that a bit odd, since
> that seemed something every test has to do, so I checked what was
> available in BaseJDBCTestCase since GroupByExpressionTest inherits
> from it. When I didn't find anything I decided to use those that I had
> copied.
>
> I ended up making quite a few modifications to them while writing the
> test because I needed additional functionality to debug the failures I
> saw during development.
>
> Now, I absolutely see the value in having such utilites in a location
> where it can be shared by all tests, but do we really need to spread
> these utilities to so many places? Makes it hard for someone who isn't
> a JUnit wizard when you can't find tests that actually follow
> "established best practices"...
I think that's always an issue, finding the best method for the job. I
don't think putting them in a single class is the best solution. JUnit
doesn't do that, and neither does the JDK. There are a couple of ways to
find methods:
1) Look at the javadoc, Andrew recently made the testing javadoc be
built nightly:
http://db.apache.org/derby/javadoc/testing/
Focus on the org.apache.derbyTesting.junit package
2) Ask a question on derby-dev, allows other to help out and see the
solution. I highly recommend this approach.
3) Look at other tests, if a test seems to have its own method that
you think should be a common utility, make it a common method, and/or
ask derby-dev why the test has its own version.
And of course, once you have found the method if you can think of any
improvement in javadoc, wiki pages etc. that would help the next person
find the method, then make the improvement.
> Well, I tried using the asserts from JDBC, but why don't they follow
> the standard JUnit assert pattern, (assertX(String message, T
> expected, T returned))? This isn't just a style issue, in this test I
> have relied heavily on passing context information, such as loop
> indices and prepared statement parameters, down to the actual assert
> method so that it gets displayed when something fails (also borrowed
> from GroupByExpressionTest). That info will just be lost if I switch
> to JDBC.assertFullResultSet...
So improve the utility methods. The standard JUnit Assert methods have
versions that take no message and those that take a message, so there's
no difference here apart from no-one has scratched the itch to add a
version with a passed in message. No-one is saying the current Derby
junit setup is perfect, it's a work in progress, as people find needs
they implement utility methods.
One comment on the loop indices in assert messages. This style of code
can be expensive at runtime:
for (int i = 0; i < count; i++) {
assertEquals("Testing at iteration " + i, f(i), g(i));
}
The issue is the message will be created 'count' times even though it is
never used. This takes up cpu time and garbage collection time.
In the Derby sanity code this style was a problem, causing extended test
running time, so the encouraged practice (for Derby sanity checks) is
if (SanityManager.ASSERT)
{
if (X != Y)
SanityManager.THROWASSERT("Value should be " + X + " is " + Y);
}
rather than
if (SanityManager.ASSERT)
{
SanityManager.ASSERT(X == Y, "Value should be " + X + " is " + Y);
}
I'm not sure what should be the best practice for JUnit tests should be,
but based upon past experience when I see a loop with a complex
iteration specific assert message I worry about execution time.
In some cases the message can be fixed, e.g. this coding style should
not be encouraged:
assertEquals("Expected " + X + " to be the equal to " + Y, X, Y);
This is because the JUnit mechanism will print out the value of X and Y,
so no need to repeat it in a generated message.
Dan.
Re: [jira] Commented: (DERBY-827) Performance can be improved by
re-using language ResultSets across Activation executions.
Posted by Army <qo...@gmail.com>.
Army wrote:
>
> Would
>
> JDBC.assertDrainResults(rs, 0);
>
> solve this particular problem? If you're expecting an empty result set
> then when you "drain" it you should get 0 rows, so wouldn't this be an
> appropriate check?
Oh, and I just noticed there's also:
JDBC.assertEmpty(rs);
which does the same thing ;)
Also, in your earlier email you wrote:
> why don't [the JDBC.assert methods] follow the standard JUnit assert
> pattern, (assertX(String message, T expected, T returned))?
When I look at JDBC.assertFullResultSet(...), I see three assertions:
// Assert that we have the right number of columns.
Assert.assertEquals("Unexpected column count:",
expectedRows[0].length, rsmd.getColumnCount());
assertRowInResultSet(rs, rows + 1,
expectedRows[rows], allAsTrimmedStrings);
// And finally, assert the row count.
Assert.assertEquals("Unexpected row count:", expectedRows.length, rows);
Is there something about these that does not follow the pattern you mention?
Sorry if I'm missing something obvious...
Army
Re: [jira] Commented: (DERBY-827) Performance can be improved by
re-using language ResultSets across Activation executions.
Posted by Army <qo...@gmail.com>.
Dyre.Tjeldvoll@Sun.COM wrote:
>
> When I tried letting my assertResultSet call
> JDBC.assertFullResultSet() I got ArrayIndexOutOfBoundsExceptions all
> over the place since I use the assert method to test empty resultsets
> against zero-length Object arrays...
Would
JDBC.assertDrainResults(rs, 0);
solve this particular problem? If you're expecting an empty result set then
when you "drain" it you should get 0 rows, so wouldn't this be an appropriate check?
Army
Re: [jira] Commented: (DERBY-827) Performance can be improved by
re-using language ResultSets across Activation executions.
Posted by Dy...@Sun.COM.
"Knut Anders Hatlen (JIRA)" <ji...@apache.org> writes:
> [ https://issues.apache.org/jira/browse/DERBY-827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12477237 ]
>
> Knut Anders Hatlen commented on DERBY-827:
> ------------------------------------------
>
> Thanks for writing the test, Dyre. I think it is a valuable addition
> to the test suite. I committed rsfromps.v1 with revision 513679.
>
> Some small comments:
>
> - assertRow() and assertResultSet() duplicate functionality that is
> already implemented in JDBC.assertRowInResultSet() and
> JDBC.assertFullResultSet(). Perhaps they can be reused?
A couple of things about this. When I started working on this test I
started by copying lang.GroupByExpressionTest since that seemed like a
pretty standard lang JUnit test. I observed that it had its own
methods for verifying that result sets. I found that a bit odd, since
that seemed something every test has to do, so I checked what was
available in BaseJDBCTestCase since GroupByExpressionTest inherits
from it. When I didn't find anything I decided to use those that I had
copied.
I ended up making quite a few modifications to them while writing the
test because I needed additional functionality to debug the failures I
saw during development.
Now, I absolutely see the value in having such utilites in a location
where it can be shared by all tests, but do we really need to spread
these utilities to so many places? Makes it hard for someone who isn't
a JUnit wizard when you can't find tests that actually follow
"established best practices"...
Well, I tried using the asserts from JDBC, but why don't they follow
the standard JUnit assert pattern, (assertX(String message, T
expected, T returned))? This isn't just a style issue, in this test I
have relied heavily on passing context information, such as loop
indices and prepared statement parameters, down to the actual assert
method so that it gets displayed when something fails (also borrowed
from GroupByExpressionTest). That info will just be lost if I switch
to JDBC.assertFullResultSet...
When I tried letting my assertResultSet call
JDBC.assertFullResultSet() I got ArrayIndexOutOfBoundsExceptions all
over the place since I use the assert method to test empty resultsets
against zero-length Object arrays...
--
dt