575 event registry bugs #576

pnstickne · 2019-11-19T06:57:02Z

"Works better". Pre-work to make #554 easier, as in 'not going to run into these issues'.

This will be part of #554 and as it contains trace-related bugs still.

fixed (apparent?) initialization ordering issue where events add during initialization could not be found later in certain cases (differed by example project).
fixed cross-access of iterators
a bunch of updates for consistency, responsibility hiding, etc.

Fixes #575

src/vt/trace/trace.cc

src/vt/trace/trace_containers.cc

pnstickne · 2019-11-19T07:00:06Z

src/vt/trace/trace_containers.h

-// Use static template initialization pattern to deal with ordering issues with
-// auto-registry
-template <typename = void>
+// Container types are created-as-needed, which occurs during


"After making these changes, events are found in the programs that don't crash". As such, I suspect it's due to an initialization ordering issue.

(Where 'these changes' are specifically related to returning a new'ed pointer instead of a reference to a static object created during initialization somewhere..)

With your changes, is all this still happening at static init time? It must execute then for correctness, except for the user's override setTraceName, if the user desires to override the given name.

src/vt/trace/trace_containers.h

src/vt/trace/trace_event.cc

src/vt/trace/trace_registry.cc

src/vt/trace/trace_containers.cc

src/vt/trace/trace.cc

src/vt/registry/auto/auto_registry_impl.h

src/vt/trace/trace_registry.cc

lifflander · 2019-11-21T19:19:38Z

src/vt/trace/trace_containers.h

-// Use static template initialization pattern to deal with ordering issues with
-// auto-registry
-template <typename = void>
+// Container types are created-as-needed, which occurs during


With your changes, is all this still happening at static init time? It must execute then for correctness, except for the user's override setTraceName, if the user desires to override the given name.

lifflander · 2019-11-21T19:21:28Z

src/vt/trace/trace_containers.h

 class TraceContainers {
 public:
-  static TraceContainerEventClassType& getEventTypeContainer(){
-    return event_type_container;
+  static TraceContainerEventClassType* getEventTypeContainer(){


What is the motivation of hoisting this to a pointer?

This could probably also be done with initialization of a non-pointer type (and separate variable capturing initialization state). Main change is it allows detection of initialization.

if (not event_container_intiailized_) { event_container_ = EventContainerType{}; // explicit creation ordering event_container_intiailized_ = true; } return event_container_;

However, even then my concern was it (event_container_) might be re-initialized later on if it was ordered after handlers during dynamic initialization.

lifflander · 2019-11-21T19:22:10Z

src/vt/trace/trace_containers.cc

+
+namespace vt { namespace trace {
+
+/*static*/ TraceContainerEventClassType*


I think you should initialize this variable to nullptr

Without the assignment it's guaranteed to be assigned all-zeros in the STATIC initialization. It's not until C++17 that assignment (in certain cases) is done during static initialization - and I wanted to be sure of not dynamic-overwrites-static initialization.

(I had it first assigning to nullptr and then removed it - a comment would probably be useful.)

@lifflander It looks like a unique_ptr would still work with the default ctor (as it is constexpr); don't quite know what C++ level guarantees constexpr as static non-local initialization. Would still be returned as a raw ptr, accessed from get to usages.

https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr

https://en.cppreference.com/w/cpp/language/initialization (really: https://en.cppreference.com/w/cpp/language/constant_initialization)

Constant initialization is performed after (until C++14)instead of (since C++14) zero initialization of the static and thread-local objects and before all other initialization. Only the following variables are constant initialized:

.. Static or thread-local object of class type that is initialized by a constructor call, if the constructor is constexpr ..

Also then a shared_ptr: constexpr shared_ptr() noexcept;.

Violated by unordered_map: https://en.cppreference.com/w/cpp/container/unordered_map/unordered_map

lifflander · 2019-11-21T19:24:26Z

src/vt/messaging/active.cc

  if (deliver) {
-    EventType event = no_event;
-
    sendMsgBytesWithPut(dest, base, msg_size, send_tag);


I think there was a bug here before, or maybe the code change. I thought that sendMsgBytesWithPut should return an event. I think the code was refactored a long time ago not to, but this code didn't get updated.

lifflander · 2019-11-21T19:25:52Z

src/vt/trace/trace.cc

-  } else {
-    return false;
-  }
+  return ArgType::vt_trace


Nice cleanup!

lifflander · 2019-11-21T19:27:48Z

src/vt/trace/trace_containers.h

-    return event_type_container;
+  static TraceContainerEventClassType* getEventTypeContainer(){
+    if (not event_type_container_)
+      event_type_container_ = new TraceContainerEventClassType();


I don't see any delete almost sure that this leaks memory. Why are you using a manually allocated object? I would steer away from this. Ideally, C++ code should never include a new

Never deleted. Global statics used in when writing the trace at the end of the program. Used when events are written - could update the naming to reflect that it's purposeful, or add a comment. (In all likeliness, this is only/almost exclusively updated during DYNAMIC initialization, with the exception of setTrace..)

static_lifetime_events_ (eg.)

nlslatt · 2019-11-25T17:02:48Z

@lifflander I don't know if it's related to this PR or not (I'm using beta.4 plus this PR on Mutrino), but I'm seeing an entry method hidden by Projections because there are no tracing events associated with it, even though it should have run many times. I'm specifically talking about Frontend::requestFields, which is called unconditionally by Backend::updateFieldSolutions (which itself does appear in the traces many times).

lifflander · 2019-11-25T17:11:57Z

@lifflander I don't know if it's related to this PR or not (I'm using beta.4 plus this PR on Mutrino), but I'm seeing an entry method hidden by Projections because there are no tracing events associated with it, even though it should have run many times. I'm specifically talking about Frontend::requestFields, which is called unconditionally by Backend::updateFieldSolutions (which itself does appear in the traces many times).

Was this also happening before 575 was being used?

- Cause an assert validation soon instead of writing to log file on process exit.. so the culprits can be better targeted.

- Busy finger work

- This will make future work simpler. Also no point dragging along even tiny costs through other TUs.

- An event ID of 0 has meaning of "no event" in various contexts. While unlikely this COULD result in missed traces. (Does not address the issue of duplicate trace ID's being possible.)

- A little bit of shifting now to help a little bit more in the future.

- All banner, no runtime logic. Now has it's own little home.

- One function doing too much and mixing stuff. - Also fixes code to return 'exit code'; ie. 0 = success, other = exit.

- All arguments must be accounted for and correctly parsed. This mitigates "oops" cases when arguments are not correctly handled by appropriate domain. To supply MPI_Init args, use: `--vt_mpi_args mpiarg1 mpiarg2..` To supply user args (returned up through initialize): `-- userarg1 userarg2 ..` To switch back to VT args, use `--vt_args` - Also supports 'auto pass-through' args for historic reasons. That is, any arguments before a "--vt_*" arg or "--help" are automatically classified as a pass-through arguments. `1 2 3 --vt_no_color` is the same as `--vt_no_color -- 1 2 3` while `--vt_no_color 1 2 3` is invalid (left in vt-arg context). Most notable, short vt args like '-c' or '-q' do NOT initiate vt-arg mode and will subject to pass-through behavior. args mergeup

- VT now terminates immediately on non-accounted-for arguments; and when --help is requested. Before args could be 'like whatever' and the help banner just flew through the terminal. - Mitigates / controls some of arg-ref behavior, builds a clean MPI_Init argument set, and removes confusing user_argc/user_argv. - Uses the new exposed prog_name which is also guaranteed to no contain extra pathing info..

- Banner message includes additional useful information about arguments that are passed downsteam. Arguments passed downsteam are not used by VT and coupled with the stricter argument checking.. should be more friendly! - Banner includes the program name (just-file-name and any more complete path passed down from mpirun).

- A little bit of shifting now to help a little bit more in the future.

- All calls now go through sync(), instead of some through there and some through MPI_Barrier.

- If any more is added, it should be skipped as parsing has been trivially rejected at the lower level. (In the future there may be more immediate post-parsing actions.)

- Now generates an MPI_Init/MPI_Abort sandwich to ensure basic contract of MPI. Output is only emitted during these times; the programs still terminate immediately thereafter.

- Help is now "--vt_help" - Removed short -c/-n/-q options for output. Use the full names like "--vt_quiet" instead.

- To avoid excessive spam, only the first node now emits a message. The responsibility is also pushed out. If the abort was caused by "help" the message is now written to stdout (instead of stderr) and the result code is 0. This is more uniform with many CLI utilities as the displaying of the help message, which was requested, is successful.

- Now the parsing and configuration are separate types, even if still sharing the same cc file for declarations. The primary reason is to reduce header-bleed of implementation related types (such as vector<int,string>). This also force uniform access to ArgConfig for all.. Also makes future non-static conversion of ArgParse much more.. lucrative. (eg. supply result message as member instead of returning outward)

- Added description and synopsis covering argument modes, switching between modes, and usages of such.. - Also unifies naming including that in banner messages. - "VT args" - used by VT configuration; --vt_* - "Application args" - supplied down-stream as argv - "MPI args" - used for MPI Init

- std::_Exit is a little more 'abrupt' than C's exit in that it does less flushing and will not call registered at-exit's.. I guess, consistency is better? Maybe.