Re: Process Tracing proposal

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
>>
>> The callbacks in the proposed API currently get their parameters by
>> pointer to non-const. I would prefer to pass events by const
>> reference and processes via an sc_process_handle param:
>>
>> void event_triggered( sc_event const & );
>> void method_started( sc_process_handle );
>>
>> Rationale:
>> - The event callbacks should not mess with the event right? Calling
>> ev->cancel() from event_triggered() may confuse the simulator.
>> - sc_process_b is not part of IEEE 1666-2005. Processes should be
>> accessed via the handle class (which can safely passed by value).
>
> We agree that the parameters should be const. However the tracer is
> expected to work with tables keyed by pointers to sc_events. Therefore,
> we would prefer passing pointers to sc_events, rather than references.
> This will also preclude copying sc_events on the fly.

  I see. In this case, passing pointers to 'const sc_event' might
indeed be the easiest way to go (instead of adding some kind of explicit
sc_event_handle).

> 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.

>> 4. Registering/unregistering an sc_process_tracer
>>
>> add|remove_process_tracer in class sc_simcontext is an implementation
>> artefact, right? I think, the registering/unregistering should be
>> done in the constructor/destructor of the tracer base class
>> automatically.
>>
>>> * All tracers, which were added to the sc_simcontext,
>>> * and not removed before the end of simulation, will be deleted by
>>> * sc_simcontext.
>>
>> This should NOT be the case, as it obliterates ownership relations
>> between the creator of the tracer and the simulation kernel.
>>
>
> We were eyeing class sc_trace_file and method add_trace_file() as an
> example of an analogous functionality :).
> We added a possibility to remove the tracer, which is an
> improvement, in our view.

  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 );

> 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?

>> 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.

> 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

-- 
Philipp A. Hartmann
Hardware/Software Design Methodology Group
OFFIS
R&D Division Transportation | FuE-Bereich Verkehr
Escherweg 2 · 26121 Oldenburg · Germany
Phone/Fax: +49-441-9722-420/282 · PGP: 0x9161A5C0 · http://www.offis.de/
-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.