Return-Path: Delivered-To: apmail-harmony-dev-archive@www.apache.org Received: (qmail 1536 invoked from network); 1 Sep 2008 05:20:14 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 1 Sep 2008 05:20:14 -0000 Received: (qmail 39468 invoked by uid 500); 1 Sep 2008 05:20:11 -0000 Delivered-To: apmail-harmony-dev-archive@harmony.apache.org Received: (qmail 39439 invoked by uid 500); 1 Sep 2008 05:20:11 -0000 Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@harmony.apache.org Delivered-To: mailing list dev@harmony.apache.org Received: (qmail 39428 invoked by uid 99); 1 Sep 2008 05:20:11 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 31 Aug 2008 22:20:11 -0700 X-ASF-Spam-Status: No, hits=-0.0 required=10.0 tests=SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of wuyuehao@gmail.com designates 209.85.142.184 as permitted sender) Received: from [209.85.142.184] (HELO ti-out-0910.google.com) (209.85.142.184) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 01 Sep 2008 05:19:14 +0000 Received: by ti-out-0910.google.com with SMTP id y6so1051467tia.18 for ; Sun, 31 Aug 2008 22:19:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to :subject:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=GQwICM/y+aER7t6fu7lYKgwN3oQcyeuosEn0h2Z7FkI=; b=g7q1ngvc2vhtzTzdWw1ZjNDJUg7VMejYOpM5t+PxMlngJyo5CyWTdwiK4uY3cFuW75 AhpPRUrPg+t7dXxzyDF0BZdygVHDU5hk7IPmKGkqKjb2k1qxIvxeRdNYUOTHmvoAeizR gz502m8Nu4lQ04rziSizJHnuL+DH1+TC1mMbA= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=TJMDW8rlKZ93OBo2e1xAFTf1JVZycfLzCJ8pD382xQplEml7uwu9zym5eRyOd7KppV Iw2ewz3/x1n2qdQWnjR+fQ/Afr6fAi65Ke17KvJFbmMO3Eo0ow7ysuw+2n75UIpQNMxO XHM/rBNAwGJOGPnk1fG5FdgDppHHQz7CCSOys= Received: by 10.110.46.3 with SMTP id t3mr7170347tit.30.1220246383800; Sun, 31 Aug 2008 22:19:43 -0700 (PDT) Received: by 10.110.20.10 with HTTP; Sun, 31 Aug 2008 22:19:43 -0700 (PDT) Message-ID: <211709bc0808312219q742fd8d6h4bf2a414b73064fa@mail.gmail.com> Date: Mon, 1 Sep 2008 13:19:43 +0800 From: "Tony Wu" To: dev@harmony.apache.org Subject: Re: svn commit: r689001 - in /harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql: Date.java Time.java Timestamp.java In-Reply-To: <3b3f27c60808291922m792cd3e2nbf7137e79af9e6e6@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20080826085957.F268C23889C4@eris.apache.org> <3b3f27c60808271433u6fe6775ahafa55298f6a6be1f@mail.gmail.com> <94d710af0808271828j4d288483g570a94ffaa8fa181@mail.gmail.com> <3b3f27c60808272006i41182de8p739d108d936b42f@mail.gmail.com> <94d710af0808272020s2854f173p5d612a3e60e9d117@mail.gmail.com> <3b3f27c60808280917k3cad944frfe37494431c2fcca@mail.gmail.com> <211709bc0808281946y2acfcd37lb61284e3872d7729@mail.gmail.com> <3b3f27c60808291922m792cd3e2nbf7137e79af9e6e6@mail.gmail.com> X-Virus-Checked: Checked by ClamAV on apache.org I reviewed these 3 tests. It's true that locales other than GMT have been covered in the TimeTest and TimeStampTest but not in DateTest. And the comments in the testToString is confusing ... Also some testcases in TimestampTest have dependency to each other. Since other locales have been covered by testToString, I just explicity set the default locale to GMT at the beginning of these testcases. I've corrected them at r690847. On Sat, Aug 30, 2008 at 10:22 AM, Nathan Beyer wrote: > But, the tests are invalid, correct? At a minimum, all of the TimeZone > code in the tests is unnecessary. > > I'm not trying to insinuate or infer that the patch is wrong. I just > like to look at each code change in a larger context. In this case, > the tests are still passing, but upon closer inspection of the tests, > they seem to be invalid or, as mentioned above, they have unnecessary > comments and seemingly invalid comments. > > -Nathan > > On Thu, Aug 28, 2008 at 9:46 PM, Tony Wu wrote: >> I think so. The format has been explicitly specified by spec, such as >> yyyy-mm-dd hh:mm:ss.fffffffff >> >> Thus it should be locale independent and this patch is valid. >> >> On Fri, Aug 29, 2008 at 12:17 AM, Nathan Beyer wrote: >>> I looked at the test in further detail and they seem okay. The tests >>> seem odd though, as they set TimeZone values and mention things like >>> TimeZone affecting the output. That shouldn't be the case though, >>> correct? The formats are all in UTC/GMT, correct? >>> >>> -Nathan >>> >>> On Wed, Aug 27, 2008 at 8:20 PM, Sean Qiu wrote: >>>> As Regis said, there exist tests for these toString method, which, >>>> IMHO, are covering this modfication. >>>> Shall we add more tests? >>>> >>>> 2008/8/28 Nathan Beyer : >>>>> I was referring to a functional test, not a performance test. >>>>> >>>>> >>>>> >>>>> On 8/27/08, Sean Qiu wrote: >>>>>> Here is a small test from Regis: >>>>>> >>>>>> ---------- >>>>>> int count = 5000; >>>>>> >>>>>> for (int i = 0; i < count; ++i) { new Date(new >>>>>> java.util.Date().getTime()).toString(); } >>>>>> >>>>>> Date date = new Date(new java.util.Date().getTime()); >>>>>> long start = System.currentTimeMillis(); >>>>>> >>>>>> for (int i = 0; i < count; ++i) { date.toString(); } >>>>>> long end = System.currentTimeMillis(); >>>>>> System.out.println("Invoke Date.toString() " + count + " times, cost: >>>>>> " + (end - start)); >>>>>> >>>>>> Time time = new Time(new java.util.Date().getTime()); >>>>>> start = System.currentTimeMillis(); >>>>>> for (int i = 0; i < count; ++i) { time.toString(); } >>>>>> end = System.currentTimeMillis(); >>>>>> System.out.println("Invoke Time.toString() " + count + " times, cost: >>>>>> " + (end - start)); >>>>>> >>>>>> Timestamp timestamp = new Timestamp(new java.util.Date().getTime()); >>>>>> start = System.currentTimeMillis(); >>>>>> for (int i = 0; i < count; ++i) { timestamp.toString(); } >>>>>> end = System.currentTimeMillis(); >>>>>> System.out.println("Invoke Timestamp.toString() " + count + " times, >>>>>> cost: " + (end - start)); >>>>>> ------------- >>>>>> >>>>>> the first loop, give time for jvm to warm up >>>>>> >>>>>> Below data compare the two implementations: >>>>>> >>>>>> before the patch: >>>>>> Invoke Date.toString() 5000 times, cost: 6757 >>>>>> Invoke Time.toString() 5000 times, cost: 7699 >>>>>> Invoke Timestamp.toString() 5000 times, cost: 2527 >>>>>> >>>>>> after the patch: >>>>>> Invoke Date.toString() 5000 times, cost: 84 >>>>>> Invoke Time.toString() 5000 times, cost: 95 >>>>>> Invoke Timestamp.toString() 5000 times, cost: 272 >>>>>> >>>>>> >>>>>> We can gain obvious improvement. >>>>>> >>>>>> 2008/8/28 Nathan Beyer : >>>>>>> Are there any associated test cases with this change? On a quick >>>>>>> cursory look, I didn't see any existing tests. Did I miss them? If >>>>>>> not, we need to start requiring some test cases for "simple >>>>>>> improvements" like this to continue functional correctness. >>>>>>> >>>>>>> -Nathan >>>>>>> >>>>>>> On Tue, Aug 26, 2008 at 3:59 AM, wrote: >>>>>>>> Author: qiuxx >>>>>>>> Date: Tue Aug 26 01:59:50 2008 >>>>>>>> New Revision: 689001 >>>>>>>> >>>>>>>> URL: http://svn.apache.org/viewvc?rev=689001&view=rev >>>>>>>> Log: >>>>>>>> Apply for HARMONY-5958,([classlib][sql][performance] - improve >>>>>>>> performance of java.sql.Date/Time/Timestamp.toString()) >>>>>>>> >>>>>>>> Modified: >>>>>>>> >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Date.java >>>>>>>> >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Time.java >>>>>>>> >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Timestamp.java >>>>>>>> >>>>>>>> Modified: >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Date.java >>>>>>>> URL: >>>>>>>> http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Date.java?rev=689001&r1=689000&r2=689001&view=diff >>>>>>>> ============================================================================== >>>>>>>> --- >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Date.java >>>>>>>> (original) >>>>>>>> +++ >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Date.java >>>>>>>> Tue Aug 26 01:59:50 2008 >>>>>>>> @@ -17,8 +17,6 @@ >>>>>>>> >>>>>>>> package java.sql; >>>>>>>> >>>>>>>> -import java.text.SimpleDateFormat; >>>>>>>> - >>>>>>>> /** >>>>>>>> * A Date class which can consume and produce dates in SQL Date format. >>>>>>>> *

>>>>>>>> @@ -175,8 +173,28 @@ >>>>>>>> */ >>>>>>>> @Override >>>>>>>> public String toString() { >>>>>>>> - SimpleDateFormat dateFormat = new >>>>>>>> SimpleDateFormat("yyyy-MM-dd"); //$NON-NLS-1$ >>>>>>>> - return dateFormat.format(this); >>>>>>>> + StringBuilder sb = new StringBuilder(10); >>>>>>>> + >>>>>>>> + format((getYear() + 1900), 4, sb); >>>>>>>> + sb.append('-'); >>>>>>>> + format((getMonth() + 1), 2, sb); >>>>>>>> + sb.append('-'); >>>>>>>> + format(getDate(), 2, sb); >>>>>>>> + >>>>>>>> + return sb.toString(); >>>>>>>> + } >>>>>>>> + >>>>>>>> + private static final String PADDING = "0000"; //$NON-NLS-1$ >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Private method to format the time >>>>>>>> + */ >>>>>>>> + private void format(int date, int digits, StringBuilder sb) { >>>>>>>> + String str = String.valueOf(date); >>>>>>>> + if (digits - str.length() > 0) { >>>>>>>> + sb.append(PADDING.substring(0, digits - str.length())); >>>>>>>> + } >>>>>>>> + sb.append(str); >>>>>>>> } >>>>>>>> >>>>>>>> /** >>>>>>>> >>>>>>>> Modified: >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Time.java >>>>>>>> URL: >>>>>>>> http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Time.java?rev=689001&r1=689000&r2=689001&view=diff >>>>>>>> ============================================================================== >>>>>>>> --- >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Time.java >>>>>>>> (original) >>>>>>>> +++ >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Time.java >>>>>>>> Tue Aug 26 01:59:50 2008 >>>>>>>> @@ -17,7 +17,6 @@ >>>>>>>> >>>>>>>> package java.sql; >>>>>>>> >>>>>>>> -import java.text.SimpleDateFormat; >>>>>>>> import java.util.Date; >>>>>>>> >>>>>>>> /** >>>>>>>> @@ -180,8 +179,28 @@ >>>>>>>> */ >>>>>>>> @Override >>>>>>>> public String toString() { >>>>>>>> - SimpleDateFormat dateFormat = new SimpleDateFormat("HH:mm:ss"); >>>>>>>> //$NON-NLS-1$ >>>>>>>> - return dateFormat.format(this); >>>>>>>> + StringBuilder sb = new StringBuilder(8); >>>>>>>> + >>>>>>>> + format(getHours(), 2, sb); >>>>>>>> + sb.append(':'); >>>>>>>> + format(getMinutes(), 2, sb); >>>>>>>> + sb.append(':'); >>>>>>>> + format(getSeconds(), 2, sb); >>>>>>>> + >>>>>>>> + return sb.toString(); >>>>>>>> + } >>>>>>>> + >>>>>>>> + private static final String PADDING = "00"; //$NON-NLS-1$ >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Private method to format the time >>>>>>>> + */ >>>>>>>> + private void format(int date, int digits, StringBuilder sb) { >>>>>>>> + String str = String.valueOf(date); >>>>>>>> + if (digits - str.length() > 0) { >>>>>>>> + sb.append(PADDING.substring(0, digits - str.length())); >>>>>>>> + } >>>>>>>> + sb.append(str); >>>>>>>> } >>>>>>>> >>>>>>>> /** >>>>>>>> >>>>>>>> Modified: >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Timestamp.java >>>>>>>> URL: >>>>>>>> http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Timestamp.java?rev=689001&r1=689000&r2=689001&view=diff >>>>>>>> ============================================================================== >>>>>>>> --- >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Timestamp.java >>>>>>>> (original) >>>>>>>> +++ >>>>>>>> harmony/enhanced/classlib/trunk/modules/sql/src/main/java/java/sql/Timestamp.java >>>>>>>> Tue Aug 26 01:59:50 2008 >>>>>>>> @@ -309,62 +309,43 @@ >>>>>>>> @SuppressWarnings("deprecation") >>>>>>>> @Override >>>>>>>> public String toString() { >>>>>>>> - /* >>>>>>>> - * Use a DecimalFormat to lay out the nanosecond value as a >>>>>>>> simple >>>>>>>> - * string of 9 integers, with leading Zeros >>>>>>>> - */ >>>>>>>> - DecimalFormat decimalFormat = new DecimalFormat("0"); >>>>>>>> //$NON-NLS-1$ >>>>>>>> - decimalFormat.setMinimumIntegerDigits(9); >>>>>>>> - decimalFormat.setMaximumIntegerDigits(9); >>>>>>>> - String theNanos = decimalFormat.format(nanos); >>>>>>>> - theNanos = stripTrailingZeros(theNanos); >>>>>>>> - >>>>>>>> - String year = format((getYear() + 1900), 4); >>>>>>>> - String month = format((getMonth() + 1), 2); >>>>>>>> - String date = format(getDate(), 2); >>>>>>>> - String hours = format(getHours(), 2); >>>>>>>> - String minutes = format(getMinutes(), 2); >>>>>>>> - String seconds = format(getSeconds(), 2); >>>>>>>> - >>>>>>>> - return year + '-' + month + '-' + date + ' ' + hours + ':' + >>>>>>>> minutes >>>>>>>> - + ':' + seconds + '.' + theNanos; >>>>>>>> - } >>>>>>>> + StringBuilder sb = new StringBuilder(29); >>>>>>>> >>>>>>>> - /* >>>>>>>> - * Private method to format the time >>>>>>>> - */ >>>>>>>> - private String format(int date, int digits) { >>>>>>>> - StringBuilder dateStringBuffer = new >>>>>>>> StringBuilder(String.valueOf(date)); >>>>>>>> - while (dateStringBuffer.length() < digits) { >>>>>>>> - dateStringBuffer = dateStringBuffer.insert(0, '0'); >>>>>>>> + format((getYear() + 1900), 4, sb); >>>>>>>> + sb.append('-'); >>>>>>>> + format((getMonth() + 1), 2, sb); >>>>>>>> + sb.append('-'); >>>>>>>> + format(getDate(), 2, sb); >>>>>>>> + sb.append(' '); >>>>>>>> + format(getHours(), 2, sb); >>>>>>>> + sb.append(':'); >>>>>>>> + format(getMinutes(), 2, sb); >>>>>>>> + sb.append(':'); >>>>>>>> + format(getSeconds(), 2, sb); >>>>>>>> + sb.append('.'); >>>>>>>> + if (nanos == 0) { >>>>>>>> + sb.append('0'); >>>>>>>> + } else { >>>>>>>> + format(nanos, 9, sb); >>>>>>>> + while (sb.charAt(sb.length() - 1) == '0') { >>>>>>>> + sb.setLength(sb.length() - 1); >>>>>>>> + } >>>>>>>> } >>>>>>>> - return dateStringBuffer.toString(); >>>>>>>> + >>>>>>>> + return sb.toString(); >>>>>>>> } >>>>>>>> >>>>>>>> - /* >>>>>>>> - * Private method to strip trailing '0' characters from a string. >>>>>>>> @param >>>>>>>> - * inputString the starting string @return a string with the >>>>>>>> trailing zeros >>>>>>>> - * stripped - will leave a single 0 at the beginning of the string >>>>>>>> - */ >>>>>>>> - private String stripTrailingZeros(String inputString) { >>>>>>>> - String finalString; >>>>>>>> + private static final String PADDING = "000000000"; //$NON-NLS-1$ >>>>>>>> >>>>>>>> - int i; >>>>>>>> - for (i = inputString.length(); i > 0; i--) { >>>>>>>> - if (inputString.charAt(i - 1) != '0') { >>>>>>>> - break; >>>>>>>> - } >>>>>>>> - /* >>>>>>>> - * If the string has a 0 as its first character, return a >>>>>>>> string >>>>>>>> - * with a single '0' >>>>>>>> - */ >>>>>>>> - if (i == 1) { >>>>>>>> - return "0"; //$NON-NLS-1$ >>>>>>>> - } >>>>>>>> + /* >>>>>>>> + * Private method to format the time >>>>>>>> + */ >>>>>>>> + private void format(int date, int digits, StringBuilder sb) { >>>>>>>> + String str = String.valueOf(date); >>>>>>>> + if (digits - str.length() > 0) { >>>>>>>> + sb.append(PADDING.substring(0, digits - str.length())); >>>>>>>> } >>>>>>>> - >>>>>>>> - finalString = inputString.substring(0, i); >>>>>>>> - return finalString; >>>>>>>> + sb.append(str); >>>>>>>> } >>>>>>>> >>>>>>>> /** >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Best Regards >>>>>> Sean, Xiao Xia Qiu >>>>>> >>>>>> China Software Development Lab, IBM >>>>>> >>>>> >>>>> -- >>>>> Sent from Gmail for mobile | mobile.google.com >>>>> >>>> >>>> >>>> >>>> -- >>>> Best Regards >>>> Sean, Xiao Xia Qiu >>>> >>>> China Software Development Lab, IBM >>>> >>> >> >> >> >> -- >> Tony Wu >> China Software Development Lab, IBM >> > -- Tony Wu China Software Development Lab, IBM