Re: Process Tracing proposal

Philipp,

Below are my answers to your comments.
To sum things up:

1. Interface to processes.

You pointed out that sc_process_b is an implementation detail, so it
should not appear in the API.

Therefore, sc_process_handle should be used as a parameter to all
process tracing calls.
If the client wishes to use pointer to process, it can be extracted from
the sc_process_handle (using get_process_object()).
All the API to discover sensibility of the process should be moved to
sc_process_handle class.

2. Registering and ownership.

Again, sc_simcontext is an implementation detail, so it should not
appear in the API.

Constructor/destructor of the tracer base class can be used for
registering and unregistering it. See, however my notes on granularity
below.

I think that we can leave the ownership of the tracers to the code which
created them. Thus, the simulator need not worry about cleaning them up
at the end. Is this acceptable?

3. Event and process tracers.

You proposed to separate event and process tracers.
I think we can agree to this, as well.
So, we can have sc_process_tracer and sc_event_tracer classes.

If we take the granularity approach described below, we'll need
sc_event_trigger_tracer (with the event_triggered() callback), as well.

4. Granularity.

We may consider adding a tracer to an sc_module, thus tracing all the
processes in its hierarchy, existing and future.
Events notification can be traced similarly, using the current process
as an hierarchy point.
Triggering of events does not necessarily occur in a process context, so
there is no granularity here.

The API may be as follows:

namespace sc_core {

void sc_add_tracer(sc_process_tracer *, sc_module *);
void sc_add_tracer(sc_event_tracer *, sc_module *);
void sc_add_tracer(sc_process_tracer *); // trace all processes
void sc_add_tracer(sc_event_tracer *); // trace all events
void sc_add_tracer(sc_event_trigger_tracer *); // trace all events
void sc_remove_tracer(sc_process_tracer *, sc_module *);
void sc_remove_tracer(sc_event_tracer *, sc_module *);
void sc_remove_tracer(sc_process_tracer *);
void sc_remove_tracer(sc_event_tracer *);
void sc_remove_tracer(sc_event_trigger_tracer *);

}

5. sc_event_iterator.

I agree that it should be STL compatible. I am not sure about the
inheritance issue.
My stdc++ installation (gcc 4.3.4) has this:

     namespace __gnu_cxx {
         template<typename _Iterator, typename _Container> class
__normal_iterator {...};
         // no base class; no further specialization.
     }

     template<...>
     class vector {
     ...
         typedef __gnu_cxx::__normal_iterator<const_pointer,
vector_type> const_iterator;
     ...
     };

6. sc_event_list and static sensitivity list.

Whatever API is added in the standard for event lists should be used
here, as well.
We were only worried about having _some_ API to the information inside.

Best regards,
Alex Barabash

On 04/28/10 23:05, Philipp A. Hartmann wrote:
> Alex, Yossi,
>
> I have some clarifying comments on your response to my comments.
>
> On 27/04/10 19:03, Veller, Yossi wrote:
>
>
>>> 3. Parameter types
>>>
>>>
>> As for sc_processes, we would prefer to pass const pointers to
>> sc_process_b, but to avoid passing sc_process_handle, as this would
>> incur a dynamic cast in the constructor of the smart pointer.
>> A possible workaround would be to add a constructor with sc_process_b
>> * parameter to sc_process_handle class.
>>
> As I said, the main problem with this is that there is no
> 'sc_process_b' in IEEE 1666-2005. ;-) I would consider an efficient
> construction of the correct handle to be an implementation issue. As
> you said, the implementation could easily work around the cast internally.
>

We think that this is a valid point.
So, let us pass sc_process_handle to all the callbacks.

>>> 4. Registering/unregistering an sc_process_tracer
>>>
>>>
> I think this is also an implementation detail. The public API, which
> is defined by the standard does not contain any reference to
> 'sc_simcontext' or 'add_trace_file' either. So, do we want to have
> explicit calls to something like:
>
> my_process_tracer mpt;
> sc_add_process_tracer(&mpt );
> // ...
> sc_remove_process_tracer(&mpt );
>
>

This is also a valid point.

>> As for registering in constructor and deregistering in destructor, we
>> did not opt for this because sub-class construction may not be ready
>> by the time base class constructor is called.
>>
> Since the (un)registering of the tracer usually only needs access to
> the base type pointer, I don't see a big problem with the class not
> being fully constructed yet.
>
> An implementation just has to ensure not to call any virtual functions
> before the final constructor is completed. This could only be the case,
> if any callbacks are triggered during the construction of the tracer,
> which I would consider quite unlikely. Am I missing something?
>

I think we can agree to this. So, let us do the registration in the
constructor and unregister in the destructor. No need for separate
explicit register/unregister calls.

>
>>> 5. Tracing granularity
>>>
>>>
> [snip]
>
>>> - Would it be reasonable to split this in two interface classes
>>> sc_event_tracer, sc_process_tracer? (a particular tracer could
>>> still implement both interfaces in a single class).
>>>
>>>
> [snip]
>
>>> For convenience, I would implement the callback as no-ops in the base
>>> classes and only leave the destructor pure virtual to enforce
>>> implementation:
>>>
> [snip]
>
>>> Otherwise, every user-defined tracer needs to implement all
>>> functions, even if only a subset of callbacks are needed.
>>>
>>>
>> We feel that the granularity we propose is the smallest meaningful.
>> First of all, events are only interesting in the context of processes which send them and those they trigger.
>>
> But a user/tracer may not be interested in tracing individual events at
> all. So why pay for the virtual function calls on a tracer, that simply
> ignores all event notifications and triggers?
>
> At least the separation of a process and an event tracing interface
> would remove this overhead.
>
> struct my_process_tracer
> : sc_core::sc_process_tracer
> {
> // only process callbacks
> } pt;
> sc_register_process_tracer(&pt );
>
> struct my_full_tracer
> : sc_core::sc_process_tracer
> , sc_core::sc_event_tracer
> {
> // trace events _and_ processes
> } ft;
> sc_register_process_tracer(&ft );
> sc_register_event_tracer(&ft );
>
> Registering, of course, could still be done automatically.
>
>

I agree.

>> In addition, sc_processes may be created on the fly, and sc_events
>> are not even sc_objects (and may be created on the fly as well).
>>
> For dynamic processes, tracers could be attached hierarchically based
> on the tracers of the parent process. Similar to the hierarchical
> process reset in the process control extensions. I agree, that for
> dynamic events a similar approach is not that easy.
>
>
>> As for the need to implement many pure virtual methods, we use
>> sc_trace_file as a reference. We also think, that having pure virtual
>> methods is a good software engineering decision in this case...
>>
> The difference to sc_trace_file compared to the current situation is,
> that for a trace file no meaningful default behaviour can be defined.
> For tracing, ignoring callbacks by default is a reasonable approach.
> But this is more or less a convenience issue.
>
>
>>
>>> 8. sc_event_list | sensitivity API
>>>
>>> This is related to having a more flexible way to access event lists,
>>> as previously discussed in this group. I have the feeling, the
>>> proposed API for event lists could be sufficient, although explicit
>>> size() and maybe empty() functions for the list would be useful
>>> especially if only a forward-iterator interface is provided.
>>>
> The most important comment in this section has been the one above.
> IIRC, explicit event lists are currently under discussion. So this
> should be aligned.
>
> [snip]
>
>
>> Firstly, sc_event_iterator interface is consistent with STL
>> const_iterator for a container of const sc_event * (which is the
>> reference implementation of sc_event_list).
>>
> See my comment on sc_event_iterator below.
>
>
>> We can add method empty() for no cost. As for size() method , this
>> adds an additional constraint (or performance hit) on the SystemC
>> implementer, which we feel is not necessary.
>>
> A naïve implementation for size() would be
> std::distance( ...begin(), ...end() );
> So I don't see a reason to omit it (since an implementer could provide a
> more efficient implementation than that).
>
>
>> Similarly, we would not assume any particular implementation for the
>> static sensitivity list, thus we would like to stick with our proposal
>> for this.
>>
> If we get explicit event lists as part of the standard, I would prefer
> to have a consistent handling throughout the whole API.
>
>
>> We agree that returning dynamic sensitivity as a struct is preferable. We shall change that.
>>
>>
>>> 9. sc_event_iterator nit-pick
>>>
>>>
> [snip]
>
>> The interface we propose only specifies the required functionality.
>> Our reference implementation is:
>> typedef std::vector<const sc_event *>::const_iterator sc_event_iterator;
>> [And we prefer a pointer to const sc_event, for the reasons we mentioned above].
>>
> From my perspective, part of an STL-compatible sc_event_iterator is
> the correct integration into the STL iterator framework. This covers a
> working implementation of std::iterator_traits< sc_event_iterator>,
> providing typedefs like iterator_category, etc. This can either be
> obtained by re-using an existing iterator, inheriting from
> std::iterator< ...>, or providing an explicit specialisation. Maybe we
> could somehow add this requirement to the definition.
>
> Hopefully, that clarifies some of my previous comments.
>
> Greetings from Oldenburg,
> Philipp
>
>

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.