You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Tony Wu <wu...@gmail.com> on 2008/02/19 10:15:02 UTC
[Approval] Commit Harmony-5460
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
Re: [Approval] Commit Harmony-5460
Posted by Tony Wu <wu...@gmail.com>.
patch committed at r629320. Thank you.
On 2/20/08, Jimmy,Jing Lv <fi...@gmail.com> wrote:
> +1 :)
>
> 2008/2/19, Tony Wu <wu...@gmail.com>:
> > On 2/19/08, Alexei Zakharov <al...@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 <wu...@gmail.com>:
> > > > On 2/19/08, Alexei Zakharov <al...@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 <wu...@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 <al...@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 <wu...@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
>
--
Tony Wu
China Software Development Lab, IBM
Re: [Approval] Commit Harmony-5460
Posted by "Jimmy,Jing Lv" <fi...@gmail.com>.
+1 :)
2008/2/19, Tony Wu <wu...@gmail.com>:
> On 2/19/08, Alexei Zakharov <al...@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 <wu...@gmail.com>:
> > > On 2/19/08, Alexei Zakharov <al...@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 <wu...@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 <al...@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 <wu...@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
Re: [Approval] Commit Harmony-5460
Posted by Tony Wu <wu...@gmail.com>.
On 2/19/08, Alexei Zakharov <al...@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 <wu...@gmail.com>:
> > On 2/19/08, Alexei Zakharov <al...@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 <wu...@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 <al...@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 <wu...@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
Re: [Approval] Commit Harmony-5460
Posted by Alexei Zakharov <al...@gmail.com>.
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.
I'm +1 for your patch to be committed.
Thanks,
Alexei
2008/2/19, Tony Wu <wu...@gmail.com>:
> On 2/19/08, Alexei Zakharov <al...@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 <wu...@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 <al...@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 <wu...@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
>
Re: [Approval] Commit Harmony-5460
Posted by Tony Wu <wu...@gmail.com>.
On 2/19/08, Alexei Zakharov <al...@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 <wu...@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 <al...@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 <wu...@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
Re: [Approval] Commit Harmony-5460
Posted by Alexei Zakharov <al...@gmail.com>.
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.
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.
Alexei
2008/2/19, Tony Wu <wu...@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 <al...@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 <wu...@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
>
Re: [Approval] Commit Harmony-5460
Posted by Tony Wu <wu...@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 <al...@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 <wu...@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
Re: [Approval] Commit Harmony-5460
Posted by Alexei Zakharov <al...@gmail.com>.
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 <wu...@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
>