harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nathan Beyer" <nbe...@kc.rr.com>
Subject RE: [classlib] Approval for String patch for HARMONY-719
Date Mon, 03 Jul 2006 19:43:10 GMT

> -----Original Message-----
> From: Geir Magnusson Jr [mailto:geir@pobox.com]
>
> Nathan Beyer wrote:
> > The concern I would have about this is the scenario where it actually is
> a
> > StringBuffer, especially an empty StringBuffer. The point of the
> instanceof
> > check and delegation to the contentEquals(StringBuffer) is to maintain
> > synchronization guarantees of StringBuffer. If we call the 'length()'
> before
> > the instanceof check, those calls will take place outside of a single
> > synchronized block.
> 
> But since StringBuffer is supposed to be threadsafe, do I have to worry
> about synchronizing on the object to get a correct value of length() in
> the event that cs is a StringBuffer?

It depends if the correctness in this case is based on the value at the time
of the length method call, or for the entire contentEquals method. It's
possible for the length to be checked the first time and return one value
and then be checked the second time and return a second value and then when
length is called again in the contentEquals(StringBuffer) method a third
value, all of which could be different. If the first two checks to length
return different values, wouldn't this compromise the algorithm and possibly
making it indeterminate if the contentEquals method is assumed to be atomic
when working with a shared StringBuffer?

I'm mostly playing Devil's Advocate here, so I personally believe that the
determinism shouldn't be left up to String, but rather to code that's using
StringBuffer and String in a concurrent fashion. However, as there's
precedent set by the other StringBuffer methods, I'm inclined to follow
that, unless we determine that the precedent is not valid.

-Nathan

> 
> >
> > I wrote the original implementation of this method, so the validity and
> > correctness of this is definitely up for discussion. I based my decision
> for
> > the "single synchronization" on the implementation of the
> > String(StringBuffer) and contentEquals(StringBuffer), which provide
> > additional synchronization above that which is inherent to StringBuffer.
> >
> > Personally, I've never assumed that the synchronization of StringBuffer
> > would every be guaranteed beyond method calls to the class itself, but I
> can
> > understand the rationale for doing this, as you'd want the outcome of
> the
> > methods to be deterministic regardless of concurrency scenarios.
> 
> I'm not sure where the lack of determinism comes from for this case -
> testing if the CharSequence has a zero length - but I'm interested in
> understanding why.

> 
> geir
> 
> >
> > -Nathan
> >
> >> -----Original Message-----
> >> From: Geir Magnusson Jr [mailto:geir@pobox.com]
> >>
> >> Interesting - couldn't you promote this fix as a minor performance
> >> improvement as well to knock out would would be a pointless instanceof
> >> in the case of ""?
> >>
> >> public boolean contentEquals(CharSequence cs) {
> >>
> >>   if (cs == null) {
> >> 	throw new NPE;
> >>   }
> >>
> >>   if (cs.length() != count) {
> >>         return false;
> >>   }
> >>
> >>   if (cs.length() == 0 && count == 0) {
> >>         return true;  // since both are empty strings
> >>   }
> >>
> >>   if (cs instanceof StringBuffer) {
> >>         return contentEquals(...);
> >>   }
> >>   else {
> >>     return regionMatches(...);
> >>   }
> >> }
> >>
> >>
> >> Nathan Beyer wrote:
> >>> http://issues.apache.org/jira/browse/HARMONY-719
> >>>
> >>>
> >>>
> >>> This issue identifies a valid bug in java.lang.String and good patch
> to
> >> fix
> >>> it. Does anyone have any objections to applying this patch or any
> >> comments?
> >>>
> >>>
> >>> -Nathan
> >>>
> >>>
> >> ---------------------------------------------------------------------
> >> Terms of use : http://incubator.apache.org/harmony/mailing.html
> >> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> >> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
> >
> 
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org


---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org


Mime
View raw message