EPICS Controls Argonne National Laboratory

Experimental Physics and
Industrial Control System

2002  2003  2004  <20052006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024  Index 2002  2003  2004  <20052006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 
<== Date ==> <== Thread ==>

Subject: FW: Standard String
From: "Jeff Hill" <[email protected]>
To: "'EPICS Core Talk'" <[email protected]>
Date: Tue, 19 Jul 2005 09:00:00 -0600

-----Original Message-----
From: Jeff Hill [mailto:[email protected]] 
Sent: Tuesday, July 19, 2005 8:57 AM
To: 'Andrew Johnson'
Cc: 'Marty Kraimer'; [email protected]
Subject: RE: Standard String


Hello Andrew,

I suppose that the important distinction is that we need to design the
interfaces used by CA and the DB to manipulate strings separately from
designing the implementation of storage for strings. I tend to gravitate
towards the former seeing it as the interface that has the most impact on
our designs. 

Yes, the string storage in the database might not be implementing the
StreamPosition / StreamRead / StreamWrite interfaces, but DA and other
interfaces in the system can very much benefit from these interfaces being
present. In particular caGet / caPut / dbGet /dbPut/ dbGetLink / dbPutLink /
etc interfaces that manipulate data might benefit. Therefore, a temporary
object could be instantiated on the stack implementing the complete
interface only for the duration of calling such functions. It may turn out
to be very nice to implement symmetric interfaces for both CA and the DB so
that CA links and ordinary links are consistent.

Here is some of the design rationale behind StreamPosition / StreamRead /
StreamWrite considered in the context of what I have seen in EpicsString:

1) Defer these functionalities to the string storage implementer who knows
best how to implement them - or can create an instance of a class that can
very efficiently implement them using details known only to the
implementation. 

2) Allow numbers to be read / written from strings in their native storage
w/o requiring copying of the entire string every time.

3) Avoid passing handles to private parts off to users. For example,
"virtual EpicsBufferCreator *bufferCreator() const" in EpicsString is an
example of such a routine. The problem is that users will eventually use the
EpicsBufferCreator after the string that it references has been destroyed.
The "expose" methods in EpicsString are also suffering from this same
problem.

Here are some additional comments on EpicsString.

1) The "expose" methods do not synchronize well the block size of the
external agent with the block size managed by EpicsBuffer. So I can ask for
1024 bytes at offset nnn and receive only two octets in response. The
StreamPosition / StreamRead / StreamWrite interfaces appear to be easier to
use and are targeting directly the functionality that DA needs.

2) The epicsStringFactory methods should probably return a reference. This
avoids need for checking, checking, checking code to verify that the pointer
is nill.

3) Assuming that EpicsString becomes a pure virtual class as Marty suggested
then I don't see any reason to complicate the interface with
createBuffer/bufferCreator/destroyBuffer methods as these details can be
passed as needed into the constructor for the implementation.

4) A very important feature of StreamRead / StreamWrite is that a
PropertyCatalog with subordinate properties extensibly controls string
formatting. 

Additional comments below (in context).

> -----Original Message-----
> From: Andrew Johnson [mailto:[email protected]]
>
> I can see why you want the various methods that make up your
> StringSegment sub-interfaces, but your design features are not all
> appropriate to a concrete string class, especially inside iocCore.
> 
> The main thing that bothers me is your StreamPosition interface.  The
> position() and movePosition() methods implies that every StringSegment
> includes an iterator into the string, which I assume is used by the
> StreamRead and StreamWrite methods to indicate where their next access
> will occur. 
> It's only possible to have one such iterator per
> StringSegment, so it's not possible for two threads to access the same
> string simultaneously, even if they're both only reading from it.

Therefore two conclusions:

1) Two threads must not have read or write access to the same string
simultaneously, and therefore some sort of mutex locking is required -
nothing new here.

2) The internal state satisfying the StreamPosition interface probably needs
to inflated temporarily (probably on the stack) for the duration of the
locked access so that different threads do not share this state. This
probably implies also that a string storage implementation does not
implement StreamPosition, but that important interfaces in the system such
as CA, dbPut, dbGet, dgPutLink, dbGetLink probably do utilize a string
interface implementing StreamPosition. 

>From my perspective, one must either include the lock in the class, use
guard classes to guarantee that locking contracts are executed, or let codes
that use the string class remember what locking is required (no compiler
enforcement). 

Since, from a locking granularity perspective, a static lock in the string
implementation is too coarse, and a lock per instance is too much overhead,
then it must either be "locks managed outside the class" or "use guard
classes to guarantee that locking contracts are executed". I originally
chose "locks managed outside the class" because this is consistent with the
rest of Data Access. 

With DA, and the CA interfaces that use it, the convention is that it is
safe to use the data interfaced by DA for the duration of the callback.
However, I can definitely see the argument for a conservative design where a
compiler enforced guard object protects the DA interface - especially in a
wider context. In that situation a PropertyCatalog reference and also a
guard reference would be passed to the CA callback, and the guard provided
must be used to access the PropertyCatalog provided. 

> 
> I don't see how you can implement StringSegment::getChar() as a const
> function if it changes the read position inside the string unless you
> mark the position as mutable, which is lying to the compiler and hence
> dangerous since calling getChar() *does* have an effect on the string
> object.  

>From that perspective all data structures accessed by multiple threads
would
also need to be volatile. Recall that there are sequence points defined in
the standards that optimizers are not allowed to cross, and that any virtual
function call defines a sequence point.

IMHO the need for marking certain data members of a class as mutable is a
common practical requirement in C++ (an obvious example is an embedded
mutex). I am guessing you will discover this on your own soon enough.
Calling getChar() *does* modify the internal state of the object - it has
to, but from the larger DA perspective it would be very detramental to the
interface design to consider that to be a modification of the data being
interfaced.

Of course, the StreamPosition / StreamRead / StreamWrite interfaces could be
passed separately from the rest of the string interface, but this would just
about eliminate use of strings with templates as templates require one
entity (template argument) implementing any type that gets used. Therefore,
some temporary helper objects implementing the StreamPosition / StreamRead /
StreamWrite interfaces might be instantiated on the stack for the duration
of the call to the CA/DB function reading/writing a string.

Another alternative would be to have a function in the core string interface
that returns a reference to the StreamPosition / StreamRead / StreamWrite
interfaces, but I decided against adding a method that returns an indirect
handle to internal storage. Lock management is complex with that approach
(or at least requires a guard class in the interface), and with that
approach there is a stronger possibility of some user using a StreamPosition
/ StreamRead / StreamWrite against a target object that no longer exists. 

Also, if you eliminate StreamPosition / StreamRead / StreamWrite and the
methods in StringSegment that manipulate the current position there isn't
much left.

> 
> If the V3 IOC used StringSegment instead of char* for storing record
> names, that would means that as long as a lockset was locked, the CA UDP
> task would not be able to complete a search where the record name it's
> looking for is lower down the same hash bucket as a record in that
> lockset (since the only record-related mutex we have in V3 is in the
> lockset, so you'd have to take that before you'd be allowed to change
> the current position of the name string).
> 
> I don't think that's acceptable, and I doubt that you would either.  A
> string is not an iterator and should not contain one, since it prevents
> simultaneous access to two different parts of the same string, even from
> inside the same function.

As mentioned above, the StreamPosition / StreamRead / StreamWrite interfaces
might need to be temporarily inflated thread private state, but nevertheless
passed along through the CA and DB interfaces that manipulate strings.
Therefore, the lock set wouldn't need to be locked.

As soon as you have on line addition of records and or changes to record
names then you certainly will need to lock out the CA UDP thread at certain
times.

We all know that the CA UDP thread occupies one of the lowest priority slots
in the IOC. Therefore, the CA client library already has to react
appropriately and adapt to situations where the UDP daemon isn't getting
much CPU.

> 
> Your StringSegment is a string accessor interface with the methods that
> you want, but it's not a general purpose string. 

A general purpose string class could of course be implemented using a
private reference to the StringSegment interface. Implemented in terms of.

> It should be possible
> to create a class that implements your StringSegment interface on top of
> our final agreed string class, although I'm not sure whether that would
> be the best or most efficient approach for your code.
> 

Much more efficient compared to taking another lock, calling new, and
copying in the string. Admittedly, EpicsBuffer might eliminate the "calling
new" step above, but not the others.

> 
> To answer your questions about EpicsString:
> 
> Jeff Hill wrote:
> >
> > 3) Is EpicsString::createBuffer assigning a buffer to be used by the
> string?
> > It's confusing who manages the storage lifetime. Does ~EpicsString
> destroy
> > it with delete (making the class unusable should the buffer not be
> created
> > with new), or does the assigner destroy it? See also (3). From my
> > perspective, the implementation should decide how the string get
> allocated.
> 
> There are overloaded EpicsString::createBuffer() methods; if you pass in
> an EpicsBufferCreator we just call it's create() method, otherwise we
> punt to the EpicsBufferFactory::create() routine instead (which will
> look up the EpicsBufferCreator for the type you requested and then call
> its create() routine.
> 
> EpicsBufferCreator is a pure interface that you register with the
> factory to tell it how to create an EpicsBuffer of your particular type.
>   Each specific EpicsBufferCreator is completely in charge of storage
> management for all EpicsBuffer objects of its type, so the
> ~EpicsString() destructor calls the pbuffer->creator()->destroy() method.

Sorry, not a very well formed question. There is this function "virtual void
createBuffer(const char *bufferType, epicsInt32 capacity = 0)" in
EpicsString. Are we passing in a buffer or the name of a buffer allocator.

> 
> > 4) The "EpicsBufferCreator *bufferCreator()" seems to require that
> > EpicsBufferCreator storage management is used. IMHO the core string
> > interface should not enforce a storage management solution. For example,
> in
> > CA I would like for the string to live in protocol buffers. I already
> have a
> > storage management solution based on a free lists template that I am
> quite
> > happy with. Furthermore, I might have a string constant implementation
> that
> > just references the string constant directly throwing an exception
> should
> > some author allow a non const ref to it.
> 
> EpicsBufferCreator is just an interface, but may not be the only
> interface implemented by the derived class.  You can create a class
> which implements EpicsBufferCreator but that has additional create()
> routines that you call directly with extra arguments.  I can see that we
> need a way to create an EpicsString using an EpicsBuffer that already
> exists and contains data, good point.  However every EpicsBuffer must
> know who its EpicsBufferCreator is since that's the way we destroy the
> buffer.
> 
> > 5) With regards to epicsString::get and epicsString::put my perception
> is
> > that the StreamPosition approach is cleaner compared to passing the
> extra
> > offset argument every time?
> 
> See my argument above regarding storing any kind of state related to a
> scanning position inside the string object itself.  The get() and put()
> methods were provided as a convenience to someone who has a contiguous
> buffer; they're not really essential, as you can use expose() instead
> inside a loop to perform the same operation (see the EpicsBuffer::expose
> description for details).
> 
> > 6) epicsString::hash might be a nice idea, but given that there are
> multiple
> > ways to hash a string, and given that we can hash a string using the
> > interfaces already present, then perhaps epicsString::hash is a
> pollution of
> > the core interface. I worry that eventually we will have two different
> > implementations of hash tables using epicsString. Will there be member
> > functions hash and hashImproved?
> 
> I accept this, we probably shouldn't have hash() in there at all.
> 
> > 7) This epicsString::isEqual taking an epicsBuffer as an argument is
> IMHO
> > not architecturally clean given that A) a string isn't being compared to
> a
> > string and B) the storage management and the string are getting mixed up
> > together.
> 
> That may be a typo, probably my fault.  I think it should be taking an
> EpicsString instead.
> 
> > 8) IMHO it is best to remain consistent with the convention of the C/C++
> > standard library and use size_t for the size of things.
> 
> I wondered about that, I'd accept that change everywhere.
> 
> - Andrew
> --
> Podiabombastic: The tendency to shoot oneself in the foot.




Replies:
Re: FW: Standard String Benjamin Franksen

Navigate by Date:
Prev: Re: Standard String Marty Kraimer
Next: Re: Standard String Kay-Uwe Kasemir
Index: 2002  2003  2004  <20052006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 
Navigate by Thread:
Prev: Re: Standard String Kay-Uwe Kasemir
Next: Re: FW: Standard String Benjamin Franksen
Index: 2002  2003  2004  <20052006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 
ANJ, 02 Feb 2012 Valid HTML 4.01! · Home · News · About · Base · Modules · Extensions · Distributions · Download ·
· Search · EPICS V4 · IRMIS · Talk · Bugs · Documents · Links · Licensing ·