harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tony Wu" <wuyue...@gmail.com>
Subject Re: [Approval] Commit Harmony-5460
Date Tue, 19 Feb 2008 12:33:51 GMT
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

Mime
View raw message