Skip to content

Commit

Permalink
Merge pull request #2148 from DARMA-tasking/2147-invalid-trace-timings
Browse files Browse the repository at this point in the history
#2147: fix incorrect order of parameters for call to beginProcessing
  • Loading branch information
nlslatt authored May 23, 2023
2 parents 522945b + f8a80b5 commit b89e422
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 8 deletions.
2 changes: 1 addition & 1 deletion src/vt/context/runnable_context/trace.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ void Trace::start(TimeType time) {
from_node_ != uninitialized_destination ? from_node_ : cur_node;

processing_tag_ = theTrace()->beginProcessing(
trace_id, msg_size_, event_, from_node, idx1_, idx2_, idx3_, idx4_, time
trace_id, msg_size_, event_, from_node, time, idx1_, idx2_, idx3_, idx4_
);
} else {
processing_tag_ = theTrace()->beginProcessing(
Expand Down
4 changes: 2 additions & 2 deletions src/vt/trace/trace.cc
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ TraceProcessingTag Trace::beginProcessing(
);

if (theConfig()->vt_trace_memory_usage) {
addMemoryEvent(theMemUsage()->getFirstUsage());
addMemoryEvent(theMemUsage()->getFirstUsage(), time);
}

return TraceProcessingTag{ep, loggedEvent};
Expand Down Expand Up @@ -404,7 +404,7 @@ void Trace::endProcessing(
);

if (theConfig()->vt_trace_memory_usage) {
addMemoryEvent(theMemUsage()->getFirstUsage());
addMemoryEvent(theMemUsage()->getFirstUsage(), time);
}

// Final event is same as original with a few .. tweaks.
Expand Down
20 changes: 19 additions & 1 deletion src/vt/trace/trace_lite.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,24 @@ struct TraceLite {
return static_cast<TimeIntegerType>(time * 1e6);
}

/**
* \brief Get the number of recorded trace events
*
* \return the number of trace events
*/
std::size_t getNumTraceEvents() const {
return traces_.size();
}

/**
* @brief Get the last recorded trace event
*
* @return the last recorded trace event
*/
const LogType* getLastTraceEvent() const noexcept {
return traces_.empty() ? nullptr : &traces_.back();
}

protected:
/**
* \brief Emit a 'stop' trace for previous open event or a '[re]start' trace
Expand Down Expand Up @@ -349,7 +367,7 @@ struct TraceLite {
*
* \return computed bytes used for tracing (lower bound)
*/
std::size_t getTracesSize() const {
std::size_t getTracesSize() const noexcept {
return traces_.size() * sizeof(Log);
}

Expand Down
15 changes: 11 additions & 4 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ set_target_properties(gtest_main PROPERTIES FOLDER extern)
include(GoogleTest)
include(turn_on_warnings)

function(add_unit_test unit_test_name unit_test_files uses_mpi)
function(add_unit_test unit_test_name unit_test_files uses_mpi additional_args)
add_executable(
${unit_test_name}
${TEST_SOURCE_FILES}
Expand Down Expand Up @@ -95,6 +95,7 @@ function(add_unit_test unit_test_name unit_test_files uses_mpi)
foreach(PROC ${PROC_TEST_LIST})
gtest_add_tests(
TARGET ${unit_test_name}
EXTRA_ARGS ${additional_args}
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
TEST_SUFFIX _proc_${PROC}
TEST_PREFIX vt:
Expand All @@ -115,6 +116,7 @@ function(add_unit_test unit_test_name unit_test_files uses_mpi)
else()
gtest_add_tests(
TARGET ${unit_test_name}
EXTRA_ARGS ${additional_args}
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
TEST_SUFFIX _no_mpi
TEST_PREFIX vt:
Expand Down Expand Up @@ -167,6 +169,7 @@ foreach(SUB_DIR ${UNIT_TEST_SUBDIRS_LIST})
set(UNIT_LIST_EXTENDED "")
set(UNIT_LIST_BASIC "")
set(UNIT_LIST_NOMPI "")
set(ADDITIONAL_ARGS "")

foreach (unit_test_file ${${SUB_DIR}_UNIT_TEST_SOURCE_FILES})
#message(STATUS "Considering ${unit_test_file}")
Expand Down Expand Up @@ -200,11 +203,15 @@ foreach(SUB_DIR ${UNIT_TEST_SUBDIRS_LIST})
endif()
endforeach()

add_unit_test("${SUB_DIR}_basic" UNIT_LIST_BASIC ON)
add_unit_test("${SUB_DIR}_nompi" UNIT_LIST_NOMPI OFF)
if (SUB_DIR STREQUAL "trace")
list(APPEND ADDITIONAL_ARGS "--vt_trace")
endif()

add_unit_test("${SUB_DIR}_basic" UNIT_LIST_BASIC ON "${ADDITIONAL_ARGS}")
add_unit_test("${SUB_DIR}_nompi" UNIT_LIST_NOMPI OFF "${ADDITIONAL_ARGS}")

if (vt_build_extended_tests)
add_unit_test("${SUB_DIR}_extended" UNIT_LIST_EXTENDED ON)
add_unit_test("${SUB_DIR}_extended" UNIT_LIST_EXTENDED ON "${ADDITIONAL_ARGS}")
endif()
endforeach()

Expand Down
99 changes: 99 additions & 0 deletions tests/unit/trace/test_runnable_context_trace.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
//@HEADER
// *****************************************************************************
//
// test_runnable_context_trace.cc
// DARMA/vt => Virtual Transport
//
// Copyright 2019-2021 National Technology & Engineering Solutions of Sandia, LLC
// (NTESS). Under the terms of Contract DE-NA0003525 with NTESS, the U.S.
// Government retains certain rights in this software.
//
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are met:
//
// * Redistributions of source code must retain the above copyright notice,
// this list of conditions and the following disclaimer.
//
// * Redistributions in binary form must reproduce the above copyright notice,
// this list of conditions and the following disclaimer in the documentation
// and/or other materials provided with the distribution.
//
// * Neither the name of the copyright holder nor the names of its
// contributors may be used to endorse or promote products derived from this
// software without specific prior written permission.
//
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
// ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
// LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
// CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
// SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
// CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
// ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
// POSSIBILITY OF SUCH DAMAGE.
//
// Questions? Contact [email protected]
//
// *****************************************************************************
//@HEADER
*/

#include <vt/context/runnable_context/trace.h>

#include "test_parallel_harness.h"
#include "test_helpers.h"

#if vt_check_enabled(trace_enabled)

namespace vt { namespace tests { namespace unit {

using TestRunnableContextTrace = TestParallelHarness;

struct TraceMsg : ::vt::Message {
};

void handler_func( TraceMsg * )
{}

TEST_F(TestRunnableContextTrace, runnable_context_trace_test_1) {
if (!theTrace()->checkDynamicRuntimeEnabled())
GTEST_SKIP() << "trace tests require --vt_trace to be set";

auto msg = makeMessage< TraceMsg >();

auto handler = auto_registry::makeAutoHandler< TraceMsg, handler_func >();

HandlerManager::setHandlerTrace(handler, true);

// Give some nonsense parameters but Trace shouldn't touch them
auto t = ctx::Trace( msg, /* in_trace_event */ 7,
handler, /* in_from_node */ 3,
5, 9, 3, 2 );

// One event for the trace, one for the top open event, third if mem usage tracing is enabled
const int add_num_events = theConfig()->vt_trace_memory_usage ? 3 : 2;

auto num_events = theTrace()->getNumTraceEvents();
t.start(TimeType{3});
EXPECT_EQ(num_events + add_num_events, theTrace()->getNumTraceEvents());
auto *last_trace = theTrace()->getLastTraceEvent();
EXPECT_NE(last_trace, nullptr);
EXPECT_EQ(last_trace->type, theConfig()->vt_trace_memory_usage ? trace::eTraceConstants::MemoryUsageCurrent : trace::eTraceConstants::BeginProcessing);
EXPECT_EQ(last_trace->time, TimeType{3});

num_events = theTrace()->getNumTraceEvents();
t.finish(TimeType{7});
EXPECT_EQ(num_events + add_num_events, theTrace()->getNumTraceEvents());
last_trace = theTrace()->getLastTraceEvent();
EXPECT_NE(last_trace, nullptr);
// Counterintuitive, but we restart the open event as the last action
EXPECT_EQ(last_trace->type, trace::eTraceConstants::BeginProcessing);
EXPECT_EQ(last_trace->time, TimeType{7}); // Time should still match though
}

}}} // end namespace vt::tests::unit

#endif // vt_check_enabled(trace_enabled)

0 comments on commit b89e422

Please sign in to comment.