Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (athena.apache.org: domain of firepure@gmail.com designates
 64.233.166.179 as permitted sender)
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=a9u5MSnb4XYchtOF4KlHgmjy2RvvSJzKTm6BGNROivzM3DzfAzPFkP8VSUZlJtH5mpQQ7bcINjB6ZpeeJzbM1fkqGY4bZKjIoftEUJQ6PXDc30mgyDl47mpdkoYCK81rpLmGQOYFkO1TIgEV0+x/60CC/nUcIe3CH+l3aiIOy80=
Message-ID: <5c8e69f0802191849j65f614f3l7386d30ff4d66540@mail.gmail.com>
Date: Wed, 20 Feb 2008 10:49:51 +0800
From: "Jimmy,Jing Lv" <firepure@gmail.com>
To: dev@harmony.apache.org
Subject: Re: [Approval] Commit Harmony-5460
In-Reply-To: <211709bc0802190548o19f443f2u20d239415a6af2b5@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <211709bc0802190115q38faf439ra1a6b6125ebd29b2@mail.gmail.com>
	 <2c9597b90802190216l255482a6h5dc3585e8c56bb01@mail.gmail.com>
	 <211709bc0802190238r5e281749n723463fc86fe2bf4@mail.gmail.com>
	 <2c9597b90802190412o291dc4d2pc4277f6fb13dbad2@mail.gmail.com>
	 <211709bc0802190433m5c9c4710v3fd8d4deb3ee335c@mail.gmail.com>
	 <2c9597b90802190527w34650cdct90fed3080646c353@mail.gmail.com>
	 <211709bc0802190548o19f443f2u20d239415a6af2b5@mail.gmail.com>

+1 :)

2008/2/19, Tony Wu <wuyuehao@gmail.com>:
> On 2/19/08, Alexei Zakharov <alexei.zakharov@gmail.com> wrote:
> > Right. So my solution is to add a synchronization to all accessors of
> > this instance variable. Your solution is to create a new object. I
> > agree that your solution is probably better because it is faster. The
> > only point I do not completely agree with you is your statement that
> >
> > > Actually your proposal doesn't work
> >
> > :) Because IMO it works too.
>
> Sorry for my abruptness :)
>
> >
> > I'm +1 for your patch to be committed.
>
> That's great. Thanks.
>
> >
> > Thanks,
> > Alexei
> >
> > 2008/2/19, Tony Wu <wuyuehao@gmail.com>:
> > > On 2/19/08, Alexei Zakharov <alexei.zakharov@gmail.com> wrote:
> > > > Tony,
> > > >
> > > > I'm not sure I understand you completely. I didn't find any places in
> > > > the spec that require creation of  a new MessageFormat  instance.
> > > The caching makes it possible to share an instance of MessageFormat
> > > between several threads, which may cause the synchronization problem
> > > because the internal status is inconsistence when do formating. The
> > > instance variable used during formating operation may be modified by
> > > other thread. To solve this problem we have two choices, one is to
> > > make the formating operation be synchronized, another is to create new
> > > instance every time.
> > >
> > > > However, at the same by looking at the code I see that such
> > > > implementation of caching is inefficient by itself because
> > > > format.toPattern() has the same (or even greater) level of complexity
> > > > if compared with MessageFormat's constructor. So I tend to agree that
> > > > creation of a new instance makes sense here.
> > >
> > > yes, it's not efficient.
> > > >
> > > > Alexei
> > > >
> > > > 2008/2/19, Tony Wu <wuyuehao@gmail.com>:
> > > > > Hi, Alexei
> > > > > Actually your proposal doesn't work, the problem is that we can not
> > > > > caching a format instance here rather than the caching mechanism. Each
> > > > > call to this static method must create a new instance of MessageFormat
> > > > > (which has been done by ICU already) then do the formating in order to
> > > > > sync the access to instance variable otherwise we need synchronize the
> > > > > format method itself. Obviously creating a new instance has higher
> > > > > performance.
> > > > >
> > > > > On 2/19/08, Alexei Zakharov <alexei.zakharov@gmail.com> wrote:
> > > > > > Hi Tony,
> > > > > >
> > > > > > Don't you think that placing a synchronized block around the existing
> > > > > > code would be a more transparent fix rather than simply redirecting
> > > > > > all job to ICU? I mean something like this:
> > > > > >
> > > > > > Index: src/main/java/java/text/MessageFormat.java
> > > > > > ===================================================================
> > > > > > --- src/main/java/java/text/MessageFormat.java  (revision 628711)
> > > > > > +++ src/main/java/java/text/MessageFormat.java  (working copy)
> > > > > > @@ -446,6 +446,8 @@
> > > > > >      *                when the pattern cannot be parsed
> > > > > >      */
> > > > > >     public static String format(String template, Object... objects) {
> > > > > > +        String result;
> > > > > > +
> > > > > >         if (objects != null) {
> > > > > >             for (int i = 0; i < objects.length; i++) {
> > > > > >                 if (objects[i] == null) {
> > > > > > @@ -453,12 +455,16 @@
> > > > > >                 }
> > > > > >             }
> > > > > >         }
> > > > > > -        if (format == null) {
> > > > > > -            format = new com.ibm.icu.text.MessageFormat(template);
> > > > > > -        } else if (!template.equals(format.toPattern())){
> > > > > > -            format.applyPattern(template);
> > > > > > +        synchronized (MessageFormat.class) {
> > > > > > +            if (format == null) {
> > > > > > +                format = new com.ibm.icu.text.MessageFormat(template);
> > > > > > +            } else if (!template.equals(format.toPattern())){
> > > > > > +                format.applyPattern(template);
> > > > > > +            }
> > > > > > +            result = format.format(objects);
> > > > > >         }
> > > > > > -        return format.format(objects);
> > > > > > +
> > > > > > +        return result;
> > > > > >     }
> > > > > >
> > > > > >     /**
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Alexei
> > > > > >
> > > > > > 2008/2/19, Tony Wu <wuyuehao@gmail.com>:
> > > > > > > This is a major problem which blocks MessageFormat running on
> > > > > > > multi-thread platform. It fails because the caching in Harmony does
> > > > > > > not work with ICU4j 3.8.1. I suggest to commit this patch for M5.
> > > > > > >
> > > > > > > --
> > > > > > > Tony Wu
> > > > > > > China Software Development Lab, IBM
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Tony Wu
> > > > > China Software Development Lab, IBM
> > > > >
> > > >
> > >
> > >
> > > --
> > > Tony Wu
> > > China Software Development Lab, IBM
> > >
> >
>
>
> --
> Tony Wu
> China Software Development Lab, IBM
>


-- 

Best Regards!

Jimmy, Jing Lv
China Software Development Lab, IBM