Thread: Re: [ADMIN] High memory usage [PATCH]
JDBC folks, can you comment on this patch? Thanks. > Attached is my patch to the official 7.1.2 PreparedStatement.java class. > This seems to work quite well for me in a test case. To try to reproduce the > seen problem I will need to test all night. I'll report tomorrow. > > BTW, this is my first attempt at making a unified diff so I might have done > something wrong. If this diff doesn't apply please tell me. > > --Rainer > > > -----Original Message----- > > From: pgsql-admin-owner@postgresql.org > > [mailto:pgsql-admin-owner@postgresql.org]On Behalf Of Rainer Mager > > Sent: Wednesday, June 20, 2001 9:08 AM > > To: Tom Lane > > Cc: PostgreSQL Admin > > Subject: RE: [ADMIN] High memory usage > > > > > > I'll work on a patch but if someone has already done this I would be > > grateful. [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 --- PreparedStatement.java Sat Feb 17 01:45:00 2001 +++ PreparedStatement.java.new Wed Jun 20 12:34:57 2001 @@ -39,10 +39,12 @@ // Some performance caches private StringBuffer sbuf = new StringBuffer(); - // We use ThreadLocal for SimpleDateFormat's because they are not that - // thread safe, so each calling thread has its own object. - private ThreadLocal tl_df = new ThreadLocal(); // setDate() SimpleDateFormat - private ThreadLocal tl_tsdf = new ThreadLocal(); // setTimestamp() SimpleDateFormat + // Because SimpleDateFormat is not thread safe we create one for each + // PreparedStatemnt here AND synchronize on it for each usage. + // We can NOT use ThreadLocal because they are not freed until the thread + // completes. This would be a memory leak for long running threads. + private SimpleDateFormat df = null; + private SimpleDateFormat tsdf = null; /** * Constructor for the PreparedStatement class. @@ -90,9 +92,6 @@ * New in 7.1 - overides Statement.close() to dispose of a few local objects */ public void close() throws SQLException { - // free the ThreadLocal caches - tl_df.set(null); - super.close(); } @@ -332,14 +331,18 @@ */ public void setDate(int parameterIndex, java.sql.Date x) throws SQLException { - SimpleDateFormat df = (SimpleDateFormat) tl_df.get(); - if(df==null) { + // The df DateFormat is initialized here to delay creation until needed. + if( df == null ) { + synchronized( this ) { + if( df == null ) { df = new SimpleDateFormat("''yyyy-MM-dd''"); - tl_df.set(df); + } + } } - set(parameterIndex, df.format(x)); - + // We must synchronize here because SimpleDateFormat is not thread safe. + synchronized( df ) { + set( parameterIndex, df.format(x) ); // The above is how the date should be handled. // // However, in JDK's prior to 1.1.6 (confirmed with the @@ -351,6 +354,7 @@ // //set(parameterIndex, df.format(new java.util.Date(x.getTime()+86400000))); } + } /** * Set a parameter to a java.sql.Time value. The driver converts @@ -375,17 +379,22 @@ */ public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException { - SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get(); - if(df==null) { - df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); - tl_tsdf.set(df); + // The tsdf DateFormat is initialized here to delay creation until needed. + if( tsdf == null ) { + synchronized( this ) { + if( tsdf == null ) { + tsdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); + tsdf.setTimeZone(TimeZone.getTimeZone("GMT")); + } + } } - df.setTimeZone(TimeZone.getTimeZone("GMT")); + // We must synchronize here because SimpleDateFormat is not thread safe. + synchronized( tsdf ) { // Use the shared StringBuffer synchronized(sbuf) { sbuf.setLength(0); - sbuf.append("'").append(df.format(x)).append('.').append(x.getNanos()/10000000).append("+00'"); + sbuf.append("'").append(tsdf.format(x)).append('.').append(x.getNanos()/10000000).append("+00'"); set(parameterIndex, sbuf.toString()); } @@ -393,6 +402,7 @@ // to be identical. Pays to read the docs ;-) //set(parameterIndex,"'"+x.toString()+"'"); } + } /** * When a very large ASCII value is input to a LONGVARCHAR parameter,
my two cents worth... 1) what is the problem that this is trying to solve, I assume from the subject that it is some sort of high memory usage? 2) I am trying to understand how a statement would ever be used by more than one thread at a time? I would think that it would be impossible to share statements across threads. 3) The double locking method used is alledgedly unsafe on SMP machines http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html. Dave ----- Original Message ----- From: "Bruce Momjian" <pgman@candle.pha.pa.us> To: "Rainer Mager" <rmager@vgkk.com> Cc: "PostgreSQL jdbc list" <pgsql-jdbc@postgresql.org>; "PostgreSQL-patches" <pgsql-patches@postgresql.org> Sent: Thursday, June 21, 2001 6:23 PM Subject: [JDBC] Re: [ADMIN] High memory usage [PATCH] > > JDBC folks, can you comment on this patch? Thanks. > > > > Attached is my patch to the official 7.1.2 PreparedStatement.java class. > > This seems to work quite well for me in a test case. To try to reproduce the > > seen problem I will need to test all night. I'll report tomorrow. > > > > BTW, this is my first attempt at making a unified diff so I might have done > > something wrong. If this diff doesn't apply please tell me. > > > > --Rainer > > > > > -----Original Message----- > > > From: pgsql-admin-owner@postgresql.org > > > [mailto:pgsql-admin-owner@postgresql.org]On Behalf Of Rainer Mager > > > Sent: Wednesday, June 20, 2001 9:08 AM > > > To: Tom Lane > > > Cc: PostgreSQL Admin > > > Subject: RE: [ADMIN] High memory usage > > > > > > > > > I'll work on a patch but if someone has already done this I would be > > > grateful. > > [ Attachment, skipping... ] > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 > ---------------------------------------------------------------------------- ---- > --- PreparedStatement.java Sat Feb 17 01:45:00 2001 > +++ PreparedStatement.java.new Wed Jun 20 12:34:57 2001 > @@ -39,10 +39,12 @@ > // Some performance caches > private StringBuffer sbuf = new StringBuffer(); > > - // We use ThreadLocal for SimpleDateFormat's because they are not that > - // thread safe, so each calling thread has its own object. > - private ThreadLocal tl_df = new ThreadLocal(); // setDate() SimpleDateFormat > - private ThreadLocal tl_tsdf = new ThreadLocal(); // setTimestamp() SimpleDateFormat > + // Because SimpleDateFormat is not thread safe we create one for each > + // PreparedStatemnt here AND synchronize on it for each usage. > + // We can NOT use ThreadLocal because they are not freed until the thread > + // completes. This would be a memory leak for long running threads. > + private SimpleDateFormat df = null; > + private SimpleDateFormat tsdf = null; > > /** > * Constructor for the PreparedStatement class. > @@ -90,9 +92,6 @@ > * New in 7.1 - overides Statement.close() to dispose of a few local objects > */ > public void close() throws SQLException { > - // free the ThreadLocal caches > - tl_df.set(null); > - > super.close(); > } > > @@ -332,14 +331,18 @@ > */ > public void setDate(int parameterIndex, java.sql.Date x) throws SQLException > { > - SimpleDateFormat df = (SimpleDateFormat) tl_df.get(); > - if(df==null) { > + // The df DateFormat is initialized here to delay creation until needed. > + if( df == null ) { > + synchronized( this ) { > + if( df == null ) { > df = new SimpleDateFormat("''yyyy-MM-dd''"); > - tl_df.set(df); > + } > + } > } > > - set(parameterIndex, df.format(x)); > - > + // We must synchronize here because SimpleDateFormat is not thread safe. > + synchronized( df ) { > + set( parameterIndex, df.format(x) ); > // The above is how the date should be handled. > // > // However, in JDK's prior to 1.1.6 (confirmed with the > @@ -351,6 +354,7 @@ > // > //set(parameterIndex, df.format(new java.util.Date(x.getTime()+86400000))); > } > + } > > /** > * Set a parameter to a java.sql.Time value. The driver converts > @@ -375,17 +379,22 @@ > */ > public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException > { > - SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get(); > - if(df==null) { > - df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); > - tl_tsdf.set(df); > + // The tsdf DateFormat is initialized here to delay creation until needed. > + if( tsdf == null ) { > + synchronized( this ) { > + if( tsdf == null ) { > + tsdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); > + tsdf.setTimeZone(TimeZone.getTimeZone("GMT")); > + } > + } > } > - df.setTimeZone(TimeZone.getTimeZone("GMT")); > > + // We must synchronize here because SimpleDateFormat is not thread safe. > + synchronized( tsdf ) { > // Use the shared StringBuffer > synchronized(sbuf) { > sbuf.setLength(0); > - sbuf.append("'").append(df.format(x)).append('.').append(x.getNanos()/100000 00).append("+00'"); > + sbuf.append("'").append(tsdf.format(x)).append('.').append(x.getNanos()/1000 0000).append("+00'"); > set(parameterIndex, sbuf.toString()); > } > > @@ -393,6 +402,7 @@ > // to be identical. Pays to read the docs ;-) > //set(parameterIndex,"'"+x.toString()+"'"); > } > + } > > /** > * When a very large ASCII value is input to a LONGVARCHAR parameter, > ---------------------------------------------------------------------------- ---- > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html >
* "Dave Cramer" <Dave@micro-automation.net> wrote: | | my two cents worth... | | 1) what is the problem that this is trying to solve, I assume from the | subject that it is some sort of high memory usage? I would like to know as well. | 2) I am trying to understand how a statement would ever be used by more than | one thread at a time? I would think that it would be impossible to share | statements across threads. Quite easy. Just the same way as you share any other object across threads and according to the JDBC spec requires that the driver shoudl be thread safe. | 3) The double locking method used is alledgedly unsafe on SMP machines | http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html. Yup, double checked locking is not going to guarantee that you get properly initialized objects with the current Java spec. -- Gunnar Rønning - gunnar@polygnosis.com Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
Good questions, I'll answer what I can... > 1) what is the problem that this is trying to solve, I assume from the > subject that it is some sort of high memory usage? Yes it involves memory usage and a memory leak. Basically, the way the code was before the patch, each instantiation of a PreparedStatement created 2 new ThreadLocal objects. According to Sun's Javadocs, these objects are not freed until the thread is completed. For a single thread app or main thread of an application (in our case a server) the thread NEVER goes away. Therefore these ThreadLocal objects just keep adding up and using memory, forever. The patch removes the use of these ThreadLocals. > 2) I am trying to understand how a statement would ever be used > by more than > one thread at a time? I would think that it would be impossible to share > statements across threads. This I can't really answer. The code appeared to be written to be thread safe so I attempted to continue that. Whether thread safe for a Statement is necessary or not I can't directly say. Although, I agree with you, I can't imagine a need to use a single Statement in multiple threads. > 3) The double locking method used is alledgedly unsafe on SMP machines > http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html. Interesting. I wasn't aware of this. If question #2 is answered such that thread safe isn't necessary, then this problem goes away pretty easily. If thread safety is needed then this would have to be rewritten, I can look into doing this if you like. > Dave --Rainer
* "Rainer Mager" <rmager@vgkk.com> wrote:
|
| Interesting. I wasn't aware of this. If question #2 is answered such that
| thread safe isn't necessary, then this problem goes away pretty easily. If
| thread safety is needed then this would have to be rewritten, I can look
| into doing this if you like.
Thread safety is required by the spec. Do you have "JDBC API tutorial and
reference, 2 ed." from Addison Wesley ? This book contains a section for
JDBC driver writers and explains this issue.
regards,
Gunnar
--
Gunnar Rønning - gunnar@polygnosis.com
Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
[snip] Wouldn't it be more elegant simply to make the ThreadLocal's static (as I'd have thought was probably the intention of the original author), this would remove the possiblity of any memory leaks in a single threaded environment, and have the same affect ultimately in a multithreaded environment? Michael Stephenson
I am going to have a look at the spec in more detail to see how they expect the driver to be used within threads I am in a similar situation wrt using the driver in server and will check if the memory usage is better. Dave -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Rainer Mager Sent: June 22, 2001 12:56 AM To: Dave Cramer; Bruce Momjian Cc: PostgreSQL jdbc list Subject: RE: [JDBC] Re: [ADMIN] High memory usage [PATCH] Good questions, I'll answer what I can... > 1) what is the problem that this is trying to solve, I assume from the > subject that it is some sort of high memory usage? Yes it involves memory usage and a memory leak. Basically, the way the code was before the patch, each instantiation of a PreparedStatement created 2 new ThreadLocal objects. According to Sun's Javadocs, these objects are not freed until the thread is completed. For a single thread app or main thread of an application (in our case a server) the thread NEVER goes away. Therefore these ThreadLocal objects just keep adding up and using memory, forever. The patch removes the use of these ThreadLocals. > 2) I am trying to understand how a statement would ever be used > by more than > one thread at a time? I would think that it would be impossible to share > statements across threads. This I can't really answer. The code appeared to be written to be thread safe so I attempted to continue that. Whether thread safe for a Statement is necessary or not I can't directly say. Although, I agree with you, I can't imagine a need to use a single Statement in multiple threads. > 3) The double locking method used is alledgedly unsafe on SMP machines > http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html. Interesting. I wasn't aware of this. If question #2 is answered such that thread safe isn't necessary, then this problem goes away pretty easily. If thread safety is needed then this would have to be rewritten, I can look into doing this if you like. > Dave --Rainer ---------------------------(end of broadcast)--------------------------- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
Here is a patch which inspired by Michael Stephens that should work
Dave
-----Original Message-----
From: pgsql-jdbc-owner@postgresql.org
[mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar Rønning
Sent: June 22, 2001 10:14 AM
To: Rainer Mager
Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list
Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH]
* "Rainer Mager" <rmager@vgkk.com> wrote:
|
| Interesting. I wasn't aware of this. If question #2 is answered such
that
| thread safe isn't necessary, then this problem goes away pretty
easily. If
| thread safety is needed then this would have to be rewritten, I can
look
| into doing this if you like.
Thread safety is required by the spec. Do you have "JDBC API tutorial
and
reference, 2 ed." from Addison Wesley ? This book contains a section for
JDBC driver writers and explains this issue.
regards,
Gunnar
--
Gunnar Rønning - gunnar@polygnosis.com
Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
> Here is a patch which inspired by Michael Stephens that should work
>
> Dave
>
>
> -----Original Message-----
> From: pgsql-jdbc-owner@postgresql.org
> [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar R?nning
> Sent: June 22, 2001 10:14 AM
> To: Rainer Mager
> Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list
> Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH]
>
> * "Rainer Mager" <rmager@vgkk.com> wrote:
> |
>
> | Interesting. I wasn't aware of this. If question #2 is answered such
> that
> | thread safe isn't necessary, then this problem goes away pretty
> easily. If
> | thread safety is needed then this would have to be rewritten, I can
> look
> | into doing this if you like.
>
> Thread safety is required by the spec. Do you have "JDBC API tutorial
> and
> reference, 2 ed." from Addison Wesley ? This book contains a section for
>
> JDBC driver writers and explains this issue.
>
> regards,
>
> Gunnar
>
> --
> Gunnar R?nning - gunnar@polygnosis.com
> Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
>
[ Attachment, skipping... ]
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Patch applied. Thanks. > Here is a patch which inspired by Michael Stephens that should work > > Dave > > > -----Original Message----- > From: pgsql-jdbc-owner@postgresql.org > [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar R?nning > Sent: June 22, 2001 10:14 AM > To: Rainer Mager > Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list > Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH] > > * "Rainer Mager" <rmager@vgkk.com> wrote: > | > > | Interesting. I wasn't aware of this. If question #2 is answered such > that > | thread safe isn't necessary, then this problem goes away pretty > easily. If > | thread safety is needed then this would have to be rewritten, I can > look > | into doing this if you like. > > Thread safety is required by the spec. Do you have "JDBC API tutorial > and > reference, 2 ed." from Addison Wesley ? This book contains a section for > > JDBC driver writers and explains this issue. > > regards, > > Gunnar > > -- > Gunnar R?nning - gunnar@polygnosis.com > Senior Consultant, Polygnosis AS, http://www.polygnosis.com/ > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > [ Attachment, skipping... ] -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Barry Lind wrote: > Since this patch got applied before I got around to commenting on it, I > have submitted a new patch to address my issues with the original patch. > > The problem with the patch as applied is that is always creates the > SimpleDateFormat objects in the constructor of the PreparedStatement > regardless of whether or not they will ever be used in the > PreparedStatement. I have reverted back to the old behavior that only > creates them if necessary in the setDate and setTimestamp methods. > > I also removed the close() method. It's only purpose was to free these > two SimpleDateFormat objects. I think it is much better to leave these > two objects cached on the thread so that other PreparedStatements can > use them. (This was the intention of a patch I submitted back in > February where I was trying to remove as many object creations as > possible to improve performance. That patch as written needed to get > pulled because of the problem that SimpleDataFormat objects are not > thread safe. Peter then added the ThreadLocal code to try to solve the > performance problem, but introduced the memory leak that originated this > email thread.) I think the cost of at most two SimpleDateFormat objects > being cached on each thead is worth the benefits of less object creation > and subsequent garbage collection. > > thanks, > --Barry > > > Bruce Momjian wrote: > >> Patch applied. Thanks. >> >> >>> Here is a patch which inspired by Michael Stephens that should work >>> >>> Dave >>> >>> >>> -----Original Message----- >>> From: pgsql-jdbc-owner@postgresql.org >>> [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar R?nning >>> Sent: June 22, 2001 10:14 AM >>> To: Rainer Mager >>> Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list >>> Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH] >>> >>> * "Rainer Mager" <rmager@vgkk.com> wrote: >>> | >>> >>> | Interesting. I wasn't aware of this. If question #2 is answered such >>> that >>> | thread safe isn't necessary, then this problem goes away pretty >>> easily. If >>> | thread safety is needed then this would have to be rewritten, I can >>> look >>> | into doing this if you like. >>> >>> Thread safety is required by the spec. Do you have "JDBC API tutorial >>> and reference, 2 ed." from Addison Wesley ? This book contains a >>> section for >>> >>> JDBC driver writers and explains this issue. >>> >>> regards, >>> Gunnar >>> >>> -- >>> Gunnar R?nning - gunnar@polygnosis.com >>> Senior Consultant, Polygnosis AS, http://www.polygnosis.com/ >>> >>> ---------------------------(end of broadcast)--------------------------- >>> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org >>> >>> >> >> [ Attachment, skipping... ] >> >> > > > ------------------------------------------------------------------------ > > *** ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java.orig Sun Jun 24 21:05:29 2001 > --- ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java Sun Jun 24 21:13:15 2001 > *************** > *** 65,78 **** > this.sql = sql; > this.connection = connection; > > - // might just as well create it here, so we don't take the hit later > - > - SimpleDateFormat df = new SimpleDateFormat("''yyyy-MM-dd''"); > - tl_df.set(df); > - > - df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); > - tl_tsdf.set(df); > - > for (i = 0; i < sql.length(); ++i) > { > int c = sql.charAt(i); > --- 65,70 ---- > *************** > *** 95,111 **** > templateStrings[i] = (String)v.elementAt(i); > } > > - /** > - * New in 7.1 - overides Statement.close() to dispose of a few local objects > - */ > - public void close() throws SQLException > - { > - // free the ThreadLocal caches > - tl_df.set(null); > - tl_tsdf.set(null); > - super.close(); > - } > - > /** > * A Prepared SQL query is executed and its ResultSet is returned > * > --- 87,92 ---- > *************** > *** 343,348 **** > --- 324,333 ---- > public void setDate(int parameterIndex, java.sql.Date x) throws SQLException > { > SimpleDateFormat df = (SimpleDateFormat) tl_df.get(); > + if(df==null) { > + df = new SimpleDateFormat("''yyyy-MM-dd''"); > + tl_df.set(df); > + } > > set(parameterIndex, df.format(x)); > > *************** > *** 382,387 **** > --- 367,376 ---- > public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException > { > SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get(); > + if(df==null) { > + df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); > + tl_tsdf.set(df); > + } > df.setTimeZone(TimeZone.getTimeZone("GMT")); > > // Use the shared StringBuffer > > patch.txt > > Content-Type: > > text/plain > Content-Encoding: > > 7bit > >
resending since I sent it from the wrong email address last time. Since this patch got applied before I got around to commenting on it, I have submitted a new patch to address my issues with the original patch. The problem with the patch as applied is that is always creates the SimpleDateFormat objects in the constructor of the PreparedStatement regardless of whether or not they will ever be used in the PreparedStatement. I have reverted back to the old behavior that only creates them if necessary in the setDate and setTimestamp methods. I also removed the close() method. It's only purpose was to free these two SimpleDateFormat objects. I think it is much better to leave these two objects cached on the thread so that other PreparedStatements can use them. (This was the intention of a patch I submitted back in February where I was trying to remove as many object creations as possible to improve performance. That patch as written needed to get pulled because of the problem that SimpleDataFormat objects are not thread safe. Peter then added the ThreadLocal code to try to solve the performance problem, but introduced the memory leak that originated this email thread.) I think the cost of at most two SimpleDateFormat objects being cached on each thead is worth the benefits of less object creation and subsequent garbage collection. thanks, --Barry >> Bruce Momjian wrote: >> >>> Patch applied. Thanks. >>> >>> >>>> Here is a patch which inspired by Michael Stephens that should work >>>> >>>> Dave >>>> >>>> >>>> -----Original Message----- >>>> From: pgsql-jdbc-owner@postgresql.org >>>> [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar R?nning >>>> Sent: June 22, 2001 10:14 AM >>>> To: Rainer Mager >>>> Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list >>>> Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH] >>>> >>>> * "Rainer Mager" <rmager@vgkk.com> wrote: >>>> | >>>> >>>> | Interesting. I wasn't aware of this. If question #2 is answered such >>>> that >>>> | thread safe isn't necessary, then this problem goes away pretty >>>> easily. If >>>> | thread safety is needed then this would have to be rewritten, I can >>>> look >>>> | into doing this if you like. >>>> >>>> Thread safety is required by the spec. Do you have "JDBC API tutorial >>>> and reference, 2 ed." from Addison Wesley ? This book contains a >>>> section for >>>> >>>> JDBC driver writers and explains this issue. >>>> >>>> regards, >>>> Gunnar >>>> >>>> -- >>>> Gunnar R?nning - gunnar@polygnosis.com >>>> Senior Consultant, Polygnosis AS, http://www.polygnosis.com/ >>>> >>>> ---------------------------(end of >>>> broadcast)--------------------------- >>>> TIP 1: subscribe and unsubscribe commands go to >>>> majordomo@postgresql.org >>>> >>>> >>> >>> [ Attachment, skipping... ] >>> >>> >> >> >> ------------------------------------------------------------------------ >> >> *** >> ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java.orig >> Sun Jun 24 21:05:29 2001 >> --- ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java >> Sun Jun 24 21:13:15 2001 >> *************** >> *** 65,78 **** >> this.sql = sql; >> this.connection = connection; >> >> - // might just as well create it here, so we don't take the >> hit later >> - - SimpleDateFormat df = new >> SimpleDateFormat("''yyyy-MM-dd''"); >> - tl_df.set(df); >> - >> - df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); >> - tl_tsdf.set(df); >> - for (i = 0; i < sql.length(); ++i) >> { >> int c = sql.charAt(i); >> --- 65,70 ---- >> *************** >> *** 95,111 **** >> templateStrings[i] = (String)v.elementAt(i); >> } >> - /** >> - * New in 7.1 - overides Statement.close() to dispose of a >> few local objects >> - */ >> - public void close() throws SQLException >> - { >> - // free the ThreadLocal caches >> - tl_df.set(null); >> - tl_tsdf.set(null); >> - super.close(); >> - } >> - /** >> * A Prepared SQL query is executed and its ResultSet is returned >> * >> --- 87,92 ---- >> *************** >> *** 343,348 **** >> --- 324,333 ---- >> public void setDate(int parameterIndex, java.sql.Date x) throws >> SQLException >> { >> SimpleDateFormat df = (SimpleDateFormat) tl_df.get(); >> + if(df==null) { >> + df = new SimpleDateFormat("''yyyy-MM-dd''"); >> + tl_df.set(df); >> + } >> set(parameterIndex, df.format(x)); >> *************** >> *** 382,387 **** >> --- 367,376 ---- >> public void setTimestamp(int parameterIndex, Timestamp x) throws >> SQLException >> { >> SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get(); >> + if(df==null) { >> + df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); >> + tl_tsdf.set(df); >> + } >> df.setTimeZone(TimeZone.getTimeZone("GMT")); >> // Use the shared StringBuffer >> >> patch.txt >> >> Content-Type: >> >> text/plain >> Content-Encoding: >> >> 7bit >> >> > >
> resending since I sent it from the wrong email address last time. > > > > Since this patch got applied before I got around to commenting on it, > > I have submitted a new patch to address my issues with the original > patch. > > The problem with the patch as applied is that is always creates the > SimpleDateFormat objects in the constructor of the PreparedStatement > regardless of whether or not they will ever be used in the > PreparedStatement. I have reverted back to the old behavior that only > creates them if necessary in the setDate and setTimestamp methods. Barry, did I miss the actual patch you sent? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Yes, this is the correct approach, great advice! -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Michael Stephenson Sent: June 22, 2001 10:42 AM To: PostgreSQL jdbc list Subject: [JDBC] Re: Re: [ADMIN] High memory usage [PATCH] [snip] Wouldn't it be more elegant simply to make the ThreadLocal's static (as I'd have thought was probably the intention of the original author), this would remove the possiblity of any memory leaks in a single threaded environment, and have the same affect ultimately in a multithreaded environment? Michael Stephenson ---------------------------(end of broadcast)--------------------------- TIP 6: Have you searched our list archives? http://www.postgresql.org/search.mpl
On the thread safe issue: I am having a hard time understanding how one would share a statement across threads. I would expect that threadsafe just means don't use static buffers so that I can have multiple statements in multiple threads. I don't think it is possible to have two threads concurrently doing a query using one statement. This being said, I don't thing it is necessary to go to great lengths to lock within the statement as long as all the members within the statement are non-static. Comments? -----Original Message----- From: gunnar@polygnosis.com [mailto:gunnar@polygnosis.com] Sent: June 22, 2001 7:42 AM To: Dave Cramer Cc: Bruce Momjian; Rainer Mager; PostgreSQL jdbc list Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH] * "Dave Cramer" <Dave@micro-automation.net> wrote: | | my two cents worth... | | 1) what is the problem that this is trying to solve, I assume from the | subject that it is some sort of high memory usage? I would like to know as well. | 2) I am trying to understand how a statement would ever be used by more than | one thread at a time? I would think that it would be impossible to share | statements across threads. Quite easy. Just the same way as you share any other object across threads and according to the JDBC spec requires that the driver shoudl be thread safe. | 3) The double locking method used is alledgedly unsafe on SMP machines | http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html. Yup, double checked locking is not going to guarantee that you get properly initialized objects with the current Java spec. -- Gunnar Rønning - gunnar@polygnosis.com Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
Since this patch got applied before I got around to commenting on it, I have submitted a new patch to address my issues with the original patch. The problem with the patch as applied is that is always creates the SimpleDateFormat objects in the constructor of the PreparedStatement regardless of whether or not they will ever be used in the PreparedStatement. I have reverted back to the old behavior that only creates them if necessary in the setDate and setTimestamp methods. I also removed the close() method. It's only purpose was to free these two SimpleDateFormat objects. I think it is much better to leave these two objects cached on the thread so that other PreparedStatements can use them. (This was the intention of a patch I submitted back in February where I was trying to remove as many object creations as possible to improve performance. That patch as written needed to get pulled because of the problem that SimpleDataFormat objects are not thread safe. Peter then added the ThreadLocal code to try to solve the performance problem, but introduced the memory leak that originated this email thread.) I think the cost of at most two SimpleDateFormat objects being cached on each thead is worth the benefits of less object creation and subsequent garbage collection. thanks, --Barry Bruce Momjian wrote: > Patch applied. Thanks. > > >>Here is a patch which inspired by Michael Stephens that should work >> >>Dave >> >> >>-----Original Message----- >>From: pgsql-jdbc-owner@postgresql.org >>[mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar R?nning >>Sent: June 22, 2001 10:14 AM >>To: Rainer Mager >>Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list >>Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH] >> >>* "Rainer Mager" <rmager@vgkk.com> wrote: >>| >> >>| Interesting. I wasn't aware of this. If question #2 is answered such >>that >>| thread safe isn't necessary, then this problem goes away pretty >>easily. If >>| thread safety is needed then this would have to be rewritten, I can >>look >>| into doing this if you like. >> >>Thread safety is required by the spec. Do you have "JDBC API tutorial >>and >>reference, 2 ed." from Addison Wesley ? This book contains a section for >> >>JDBC driver writers and explains this issue. >> >>regards, >> >> Gunnar >> >>-- >>Gunnar R?nning - gunnar@polygnosis.com >>Senior Consultant, Polygnosis AS, http://www.polygnosis.com/ >> >>---------------------------(end of broadcast)--------------------------- >>TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org >> >> > > [ Attachment, skipping... ] > > *** ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java.orig Sun Jun 24 21:05:29 2001 --- ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java Sun Jun 24 21:13:15 2001 *************** *** 65,78 **** this.sql = sql; this.connection = connection; - // might just as well create it here, so we don't take the hit later - - SimpleDateFormat df = new SimpleDateFormat("''yyyy-MM-dd''"); - tl_df.set(df); - - df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); - tl_tsdf.set(df); - for (i = 0; i < sql.length(); ++i) { int c = sql.charAt(i); --- 65,70 ---- *************** *** 95,111 **** templateStrings[i] = (String)v.elementAt(i); } - /** - * New in 7.1 - overides Statement.close() to dispose of a few local objects - */ - public void close() throws SQLException - { - // free the ThreadLocal caches - tl_df.set(null); - tl_tsdf.set(null); - super.close(); - } - /** * A Prepared SQL query is executed and its ResultSet is returned * --- 87,92 ---- *************** *** 343,348 **** --- 324,333 ---- public void setDate(int parameterIndex, java.sql.Date x) throws SQLException { SimpleDateFormat df = (SimpleDateFormat) tl_df.get(); + if(df==null) { + df = new SimpleDateFormat("''yyyy-MM-dd''"); + tl_df.set(df); + } set(parameterIndex, df.format(x)); *************** *** 382,387 **** --- 367,376 ---- public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException { SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get(); + if(df==null) { + df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); + tl_tsdf.set(df); + } df.setTimeZone(TimeZone.getTimeZone("GMT")); // Use the shared StringBuffer