Bug #6225 speed issue with ssps.setLong(i, x) (new Long trashing)
Submitted: 23 Oct 2004 1:50 Modified: 20 Dec 2004 18:12
Reporter: Quartz 12h Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / J Severity:S3 (Non-critical)
Version:3.1.4b OS:Windows (win2k)
Assigned to: Mark Matthews CPU Architecture:Any

[23 Oct 2004 1:50] Quartz 12h
Description:
In
com.mysql.jdbc.ServerSidePreparedStatement
setLong(int parameterIndex, long x)
there is a possible abuse of 'new Long(x)'

We go into a great deal of effort to avoid memory trashing by using primitives and canonicalizing Long and Integers whenever possible. Seing that seams a bit trashy.

Please review if you can preserve the long (or other) primitives.
Also try to avoid (or do use thread local caching) for the BindValue object
(seen a bunch of then on addBatch or BatchedBindValues<init>).

How to repeat:
do performance analysis of setLong (or other setters) and watch BindValues instances too.
[23 Oct 2004 2:34] Mark Matthews
This can not easily be fixed, as there is no concept of a 'union' struct in Java, which means we would have to keep a long-lived instance array for _every_ primitive type, and one for the object types around, which is a scenario that has it's own problems. 

Because of the use-cases of prepared statements, and the requirements they have, it is usually not possible to naiively cache instances such as BindValues either.

Modern VMs don't have issues (when configured correctly) with short-lived temporary objects, and in fact many VM vendors actually _discourage_ the kind of caching you propose as it defeats many of the features of newer garbage collection schemes, and caching objects that aren't time-intensive to create (such as sockets or file descriptors) in many cases now _negatively_ affects performance and scalability on modern JVMs.

We've yet to see this be a performance issue in production or benchmarking (including SpecJAppserver). If you have data (benchmarks, profiling, maybe specific to your usage?) to back up your issue, we'd love to see it, and if possible add it to our suite of performance regression tests. 

As it stands now, we unfortunately haven't been able to produce or have been made aware of any data that is points out issues similar to those you are having.
[25 Oct 2004 14:34] Quartz 12h
>Modern VMs don't have issues (when configured correctly)
> with short-lived temporary objects,

I always try to test first. I'm satisfied when I see the experimental proof.

It take 6 IFs to be worth a function call; I try 'if-instanceof' just for speed.

it take few function calls to be worth a single 'new' call; I try pooling objects.

There is no JVM in the world that will prevent CPU cycles to be wasted in allocating/gc'ing an objects. If new JVMs were so good at alloc/gc, then we wouldn't get 8x speed increase by pooling linked list entries in proprietary queues code... 

As long as the memory foot print scales with number of prepared statement,
people will agree to sacrifice memory for speed. Remember I'm talking only about the prepared statement case, which is specifically adressing performance aspects.

Meanwhile, apps don't have tens of thousands of connections and/or prepared statements usually, but they do perform millions of statements per hour (in our case at least). Think of it like a stringbuffer: the char array grows as needed. A warmed up prepared statement would adjust to the needs it fulfills, reaching an optimal 'cached' state.

If the 'scenario that has it's own problems', as you wrote,  is another one, please highlight it, I might help. Up to now, I have never seen a case where caching couldn't be implemented, although some are impractical.

I will attach one test code I use. Should help.

>As it stands now, we unfortunately haven't been able to
> produce or have been made aware of any data that is
> points out issues similar to those you are having.

You 'really' need a jvmpi profiler to 'see' that issue. I used optimizeit 4.2.
with jdk 1.4.2_05.
[25 Oct 2004 14:56] Quartz 12h
Hum. It's monday allright...
For completeness:

-If apps have thousands of preparedstatements, even if each uses 1k to cache some instances, that is still just 1 meg.

-if jvm vendors don't recommend object pooling, it is to prevent having too many long lived objects, so that the full gc runs faster. It is not because they do faster allocations. This distinction is capital.

If all objects were pooled, the old generation would be full, but there wouldn't be any full gc. But given that this is not possible, I would still rather want a full gc every 5 minutes that lasts 5 second than reducing my throughput and still have the risk of a full gc pause.
[25 Oct 2004 14:59] Quartz 12h
test code (need the properties file)

Attachment: TestSQLStatements.java (text/plain), 14.53 KiB.

[25 Oct 2004 15:00] Quartz 12h
props file

Attachment: test_3.1.4b.properties (application/octet-stream, text), 741 bytes.

[25 Oct 2004 15:03] Quartz 12h
on the test code, I used setInt(). You might want to change schema/use setLong()  as needed.

(this code compares simple statements to server side prepared statement, and actually was intended to compare drivers performance)
[3 Nov 2004 2:24] Quartz 12h
refactored without wrappers

Attachment: ServerPreparedStatement.java (text/plain), 52.59 KiB.

[3 Nov 2004 2:31] Quartz 12h
once you changed the BindValue, everything goes by itself...
the 'new' cost diseappeared completely! And it could even be smaller.
see the server prepared statement just attached.

Basically, this is a simplified version of a union.
To make even more compact, we could actually keep only:
-long (for boolean,byte,short,char,int and long)
-double (for float and double)
-object (already overloaded)
the secret is in the type field.

Double check it, have fun.
Hope to see that in next connector/J.

What are the plans for 3.1.x 'gamma' connector/J?

PS:

-always use static innerclass if you don't need enclosing class

-use Object.clone() for such shallow copies (copies primitive by value) because
it is a fast jvm call to clone copy the object.

Excerpt:

	static class BindValue implements Cloneable {
		boolean isLongData; /* long data indicator */
		int bufferType; /* buffer type */
		long bindLength; /* Default length of data */
		Object objvalue; //L=genericobject, t=string
		
		byte b;//B
		short s;//S
		int i;//I
		long j;//J
		float f;//F
		double d;//D
		
		char type = 'N';//N=null, t=text, else see others fields

		public final Object clone() throws CloneNotSupportedException {
			return super.clone();
		}
	}
[3 Nov 2004 3:06] Mark Matthews
We actually already implemented something similar to your patch earlier this week (it was still in testing).

See the nightly snapshot builds starting yesterday/Sunday.

We probably won't go so far as to 'collapse' fields, just for readibility's sake. Also, you don't need to add an extra descriminator, as in your patch, as BindValues already have a type ;)
[3 Nov 2004 14:20] Quartz 12h
fix setBoolean(): type =  'Z' not 'z'. Uses BindValue copy constructor, not clone() (turn out to be faster! sorry)

Attachment: ServerPreparedStatement.java (text/plain), 52.50 KiB.

[3 Nov 2004 14:24] Quartz 12h
(Thanks for the quick feedback. We are in a perf tuning rush here, and I appreciate.)

FYI, the http://downloads.mysql.com/snapshots.php
doesn't reflect the nightly build past oct 11th... (3.1 alpha)
Where do I find source, or CVS?

Thanks again.
Quartz12h.

PS: the second ServerPreparedStatement.java fixes 1 typo (boolean type = 'z' should have been type = 'Z', and I went back to BindValue copy constructor, which is still faster than clone() native method call. My bad, sorry.)
[3 Nov 2004 15:32] Mark Matthews
Please try the snapshots site again, there was an issue with our nightly sync script. Yesterday's build is there, with the fixes in place (I just downloaded it to check).
[3 Nov 2004 17:00] Quartz 12h
Good job. Nice to see we thought of the same tricks.
But...
2 issues:

-your toString() is gonna fail: manually rename the BindValue.value to something else: it will highlight (compiler errors in IDE) the places depending on it. You will find more than what you changed.

-please, I'm begging you to use a switch/case in storeBindings() !!
;-)

Thanks!
[20 Dec 2004 18:12] Mark Matthews
Thank you for your bug report. This issue has already been fixed
in the latest released version of that product, which you can download at 
http://www.mysql.com/downloads/