RE: Process Tracing proposal

Hi Philipp,

>
> I had a quick look through your proposal. Generally speaking, I would
> like to see something like this in the next 1666 revision.

Thanks for your comments.

>
> 1. Performance impact
>
> Since the tracing calls are on the critical path in the simulator
> (every event notify/trigger, process state change), there may be
> a performance degradation even when no observers are registered.
> Do you have benchmarks on this? Should we add a preprocessor
> symbol to explicitly activate this feature, like
> SC_INCLUDE_PROCESS_TRACING?

It is our feeling that performance degradation when no observers are registered should be negligible. To prove it, we are implementing a prototype and will measure the effect. This calls are implemented under a compilation flag anyway.

>
> 2. Feedback from CCIWG
>
> This feature is closely related to the OSCI CCI working group, since
> it helps for model introspection. Although the CCIWG currently works
> on configuration, it may be reasonable to get feedback from them as
> well.

This is a good idea. I'll send this document also on the CCIWG reflector.

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

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.

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

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.

> 5. Tracing granularity
>
> The proposed tracing is a centralised mechanism for events _and_
> processes. Two questions on this:
>
> - 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).
>
> - If a model is only interested in tracing a particular subset of
> processes/events, the filtering has to be done inside the tracer.
> An interface to add a tracer to an sc_event/sc_process_handle
> explicitly could increase the flexibility here.
>
> For convenience, I would implement the callback as no-ops in the base
> classes and only leave the destructor pure virtual to enforce
> implementation:
>
> struct sc_process_tracer
> {
> virtual void event_notified( sc_event const & ) {}
> virtual ~sc_process_tracer() = 0;
> };
> inline sc_process_tracer::~sc_process_tracer(){}
>
> 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.
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).

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

> 6. sc_event callbacks, immediate notification
>
> I think, the notify_t needs another value IMMEDIATE, for immediate
> notification of events within the same delta. Here, it needs to be
> defined, when the callbacks are executed.
>
> Consider the following SC_METHOD(foo):
> {
> // do something
> ev.notify(); // immediate notification
> // do something else
> }
>
> Callbacks involved (all in the same delta cycle):
> method_started( foo );
> event_notified( ev );
> method_finished( foo );
> event_triggered( ev );
>
> What order do we expect between the event_* and the method_*
> hooks here?
>

The order is as follows:
        method_started( foo );
        event_notified( ev );
        event_triggered( ev );
        method_finished( foo );

The value of notify_t in this case will be NONE.
We propose to add an alias of IMMEDIATE for the value of NONE.

> 7. Process callbacks
>
> For the process related callbacks, a more elaborate definition of
> the execution ordering between different callbacks and relative to
> execution of process code should be defined. e.g. 'thread_resumed()'
> -> 'after resuming a thread', but before or after actually running
> it? 'after a thread was reset' -> before or after restarting it?
>
> What is 'normal termination'? How should the tracer react to
> exceptions? I think, exceptions (not only sc_report) should be
> caught and re-thrown after invoking the termination callback, but
> at least it should be defined.
>
> Are all termination callbacks guaranteed to be executed at the end
> of the simulation? Are processes "killed" then?
>

We shall add clarifications to these definitions, following the semantics of the SystemC kernel.

> 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.
>
> Are sc_event_or_list, sc_event_and_list then meant to inherit from
> sc_event_list? Should an sc_event_list know about it being an
> "and" or "or" list?
>
> The API to query the static/dynamic sensitivity looks quite ad-hoc to
> me, though. I don't really like passing references to pointers
> around and prefer clear ownership and lifetime definitions.
>
> So I would replace
> sc_event_iterator static_sensitivity_begin_iterator() const;
> sc_event_iterator static_sensitivity_end_iterator() const;
> with a single
> sc_event_list static_sensitivity() const;
>
> and
>
> trigger_t get_dynamic_sensitivity(sc_event const *&event_p,
> sc_event_list const *&event_list_p,
> sc_time &notify_time) const;
>
> with something like
>
> struct sc_sensitivity {
> trigger_t kind; // maybe move into sc_event_list?
> sc_event_list events;
> sc_time notify_time;
> };
>
> sc_sensitivity dynamic_sensitivity() const;
>
> These functions need to be added to the process-handle class, as well.
>

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

Similarly, we would not assume any particular implementation for the static sensitivity list, thus we would like to stick with our proposal for this.

We agree that returning dynamic sensitivity as a struct is preferable. We shall change that.

> 9. sc_event_iterator nit-pick
>
> - operator* should return a reference, instead of a pointer:
> (*it)->foo() feels unnatural to me, since it is an event-list, not
> an "event-handle-list".
>
> - sc_event_iterator could derive from
> std::iterator< std::forward_iterator_tag, sc_event >
> for better integration with the C++ standard library
> (maybe std::iterator< std::bidirectional_iterator_tag, sc_event > ?)
>

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

>
> Greetings from Oldenburg,
> Philipp
>

Best Regards,
Alex Barabash & Yossi Veller

> On 26/04/10 05:29, Glasser, Mark wrote:
> >
> > As I mentioned at the recent P1666 meeting, Mentor has a proposal for a
> > process tracing feature to be included in the next 1666 revision. The
> > feature is essentially a set of callbacks which are invoked as processes
> > change state. We would like to get feedback on the proposal from
> > members of this discussion group.
>
> --
> 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.