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 Tue, 04 Jul 2006 21:26:19 GMT
> -----Original Message-----
> From: Geir Magnusson Jr [mailto:geir@pobox.com]
> Nathan Beyer wrote:
> >> -----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?
> 
> 
> Based on thread/process scheduling, if the caller is constantly varying
> the value of the argument during the lifetime of the call, the above
> would also give different answers from time to time, but I'd argue they
> are perfectly determinate as we're not depending on stale data due to
> synchronization problems.
> 

I'm not sure I quite understand what you're saying. What I do know is that
if String.contentEquals(StringBuffer) is called with a non-null
StringBuffer, then the StringBuffer will not change during the execution of
that method because of the synchronization on the StringBuffer. This
behavior is currently maintained by String.contentEquals(CharSequence) if a
non-null StringBuffer is passed. A StringBuffer could be changed just prior
to the synchronization block, but all interrogation of the StringBuffer will
be guaranteed consistent while inside that block. If the StringBuffer is
interrogated without a synchronized block, then there is no guarantee that
the data returned by StringBuffer won't change and there's no guarantee that
the order of the code won't change either (the VM is free to reorder the
bytecodes).

> >
> > 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.
> 
> I still think that this is determinate code.  What I was suggesting is
> minor so I don't feel strongly either way, but I'm still not convinced
> because there are no contracts expressed about timing if the caller
> chooses to vary the contents of what's being tested in contentEquals.
> It's not about the answer being determined some fixed number of
> instructions into the method...

I agree and following that thought, it should be correct to also remove the
synchronization in the contentEquals(StringBuffer) method and the
String(StringBufffer) constructor, right? If String isn't going to guarantee
synchronization of StringBuffer on one method, it shouldn't need to do it on
any method, correct?

What I'm trying to get at is a more rudimentary assertion about the way
String is implemented. Should String's implementation know anything about
StringBuffer's possible concurrent usage? I don't think that it should,
which I think is what you're inferring as well.

> 
> I see no problem if contentEquals() gets to decide that it's not a match
> given the argument is an empty string even though that is some number of
> instructions sooner than the test if the string isn't empty.
> 
> Also, I wonder if there is a way to do this without String having to
> know about the internals of StringBuffer....
> 
> geir
> 
> 
> 
> >
> > -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
> >
> >
> >
> 
> ---------------------------------------------------------------------
> 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