Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Planner comparison and update #1061

Merged
merged 17 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ if(DEFINED ENV{WITH_GO})
include(GolangSimple)
endif()

include(FindValgrind) # Search for Valgrind

include_directories(.)
add_subdirectory( etc )
add_subdirectory( src )
Expand Down
19 changes: 19 additions & 0 deletions cmake/FindValgrind.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Look for Valgrind headers and binary.
#
# Variables defined by this module:
# Valgrind_FOUND System has valgrind
# Valgrind_INCLUDE_DIR where to find valgrind/memcheck.h, etc.
# Valgrind_EXECUTABLE the valgrind executable.
# This module appends to config.h so t5000-valgrind.t succeeds.
# We may need to change this behavior once remaining autotools
# files are removed.

find_path(Valgrind_INCLUDE_DIR valgrind HINTS ${Valgrind_INCLUDE_PATH})
find_program(Valgrind_EXECUTABLE NAMES valgrind PATH ${Valgrind_BINARY_PATH})

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(Valgrind DEFAULT_MSG Valgrind_INCLUDE_DIR Valgrind_EXECUTABLE)

if(Valgrind_FOUND)
file(APPEND config.h "#define HAVE_VALGRIND 1\n")
endif()
1 change: 1 addition & 0 deletions resource/planner/c++/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ add_library(planner_cxx STATIC
./scheduled_point_tree.cpp
./mintime_resource_tree.cpp
./planner_multi.cpp
./planner_internal_tree.cpp
./mintime_resource_tree.hpp
./planner_internal_tree.hpp
./scheduled_point_tree.hpp
Expand Down
3 changes: 2 additions & 1 deletion resource/planner/c++/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ libplanner_cxx_la_SOURCES = \
planner.cpp \
planner_multi.cpp \
scheduled_point_tree.cpp \
mintime_resource_tree.cpp
mintime_resource_tree.cpp \
planner_internal_tree.cpp

libplanner_cxx_la_CFLAGS = $(AM_CFLAGS) -I$(top_srcdir)/resource/planner/c++

Expand Down
11 changes: 11 additions & 0 deletions resource/planner/c++/mintime_resource_tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,17 @@ bool mt_resource_rb_node_t::operator< (const mt_resource_rb_node_t &other) const
return this->remaining < other.remaining;
}

bool mt_resource_rb_node_t::operator== (const mt_resource_rb_node_t &other) const
{
return this->remaining == other.remaining;
}

bool mt_resource_rb_node_t::operator!= (const mt_resource_rb_node_t &other) const
{
return !operator == (other);
}



/*******************************************************************************
* *
Expand Down
2 changes: 2 additions & 0 deletions resource/planner/c++/mintime_resource_tree.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ struct mt_resource_rb_node_t
int64_t subtree_min;
int64_t remaining;
bool operator< (const mt_resource_rb_node_t &other) const;
bool operator== (const mt_resource_rb_node_t &other) const;
bool operator!= (const mt_resource_rb_node_t &other) const;
};

template <class mt_resource_rb_node_t, class NodeTraits>
Expand Down
105 changes: 65 additions & 40 deletions resource/planner/c++/planner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,37 @@
#include "planner.hpp"


/****************************************************************************
* *
* Public Span_t Methods *
* *
****************************************************************************/

bool span_t::operator== (const span_t &o) const
{
if (start != o.start)
return false;
if (last != o.last)
return false;
if (span_id != o.span_id)
return false;
if (planned != o.planned)
return false;
if (in_system != o.in_system)
return false;
if ((*(start_p) != *(o.start_p)))
return false;
if ((*(last_p) != *(o.last_p)))
return false;

return true;
}

bool span_t::operator!= (const span_t &o) const
{
return !operator == (o);
}

/****************************************************************************
* *
* Public Planner Methods *
Expand Down Expand Up @@ -118,7 +149,7 @@
return false;
// m_p0 or o.m_p0 could be uninitialized
if (m_p0 && o.m_p0) {
if (!scheduled_points_equal (*m_p0, *(o.m_p0)))
if (*m_p0 != *(o.m_p0))
return false;
} else if (m_p0 || o.m_p0) {
return false;
Expand All @@ -132,6 +163,11 @@
return true;
}

bool planner::operator!= (const planner &o) const
{
return !operator == (o);
}

planner::~planner ()
{
// Destructor is nothrow
Expand Down Expand Up @@ -193,6 +229,30 @@
return rc;
}

int planner::update_total (uint64_t resource_total)
{
int64_t delta = resource_total - m_total_resources;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is m_total_resources guaranteed to be smaller than resource_total?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case where resource_total > m_total_resources is when the graph is being grown, and resource_total < m_total_resources means the graph is being shrunk. There aren't any guarantees on the value of resource_total except that it's nonnegative.

@jameshcorbett asked a form of this question, too, and my reasoning has been that negative values for point->remaining (e.g., when (point->remaining + delta) < 0) shouldn't cause problems because they should only happen for reservations (which will be cleared at the next schedule loop). I was thinking that adding a check for negative remaining wasn't worth the performance penalty.

That said, I don't expect update_total to be called often, and adding a guard check may prevent complicated problems with removing spans if that process isn't carefully performed upstream (i.e., in planner_c_interface).

int64_t tmp = 0;
scheduled_point_t *point = nullptr;
if (delta == 0)
return 0;
m_total_resources = static_cast<int64_t> (resource_total);
point = m_sched_point_tree.get_state (m_plan_start);
while (point) {
// Prevent remaining from taking negative values. This should
// reduce likelihood of errors when adding and removing spans.
// If the performance penalty is non-negligible we can
// investigate letting remaining take negative values.
tmp = point->remaining + delta;
if (tmp >= 0)
point->remaining = tmp;
else
point->remaining = 0;

Check warning on line 250 in resource/planner/c++/planner.cpp

View check run for this annotation

Codecov / codecov/patch

resource/planner/c++/planner.cpp#L250

Added line #L250 was not covered by tests
point = m_sched_point_tree.next (point);
}
return 0;
}

int64_t planner::get_total_resources () const
{
return m_total_resources;
Expand Down Expand Up @@ -428,25 +488,6 @@
return rc;
}

bool planner::scheduled_points_equal (const scheduled_point_t &lhs,
const scheduled_point_t &rhs) const
{
if (lhs.at != rhs.at)
return false;
if (lhs.in_mt_resource_tree != rhs.in_mt_resource_tree)
return false;
if (lhs.new_point != rhs.new_point)
return false;
if (lhs.ref_count != rhs.ref_count)
return false;
if (lhs.remaining != rhs.remaining)
return false;
if (lhs.scheduled != rhs.scheduled)
return false;

return true;
}

bool planner::span_lookups_equal (const planner &o) const
{
if (m_span_lookup.size () != o.m_span_lookup.size ())
Expand All @@ -460,23 +501,8 @@
return false;
if (this_it.first != other->first)
return false;
if (this_it.second->start != other->second->start)
return false;
if (this_it.second->last != other->second->last)
return false;
if (this_it.second->span_id != other->second->span_id)
return false;
if (this_it.second->planned != other->second->planned)
return false;
if (this_it.second->in_system != other->second->in_system)
return false;
if (this_it.second->in_system != other->second->in_system)
return false;
if (!scheduled_points_equal (*(this_it.second->start_p),
*(other->second->start_p)))
return false;
if (!scheduled_points_equal (*(this_it.second->last_p),
*(other->second->last_p)))
// Compare span_t
if (*(this_it.second) != *(other->second))
return false;
}
}
Expand All @@ -497,8 +523,7 @@
if (this_it.first != other->first)
return false;
if (this_it.second && other->second) {
if (!scheduled_points_equal (*(this_it.second),
*(other->second)))
if (*(this_it.second) != *(other->second))

Check warning on line 526 in resource/planner/c++/planner.cpp

View check run for this annotation

Codecov / codecov/patch

resource/planner/c++/planner.cpp#L526

Added line #L526 was not covered by tests
return false;
} else if (this_it.second || other->second) {
return false;
Expand All @@ -518,7 +543,7 @@
scheduled_point_t *o_pt =
o.m_sched_point_tree.get_state (o.m_plan_start);
while (this_pt) {
if (!scheduled_points_equal (*this_pt, *o_pt))
if (*this_pt != *o_pt)
return false;
this_pt = m_sched_point_tree.next (this_pt);
o_pt = o.m_sched_point_tree.next (o_pt);
Expand Down
7 changes: 5 additions & 2 deletions resource/planner/c++/planner.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ struct request_t {
/*! Node in a span interval tree to enable fast retrieval of intercepting spans.
*/
struct span_t {
bool operator== (const span_t &o) const;
bool operator!= (const span_t &o) const;

int64_t start; /* start time of the span */
int64_t last; /* end time of the span */
int64_t span_id; /* unique span id */
Expand All @@ -42,11 +45,13 @@ class planner {
planner (const planner &o);
planner &operator= (const planner &o);
bool operator== (const planner &o) const;
bool operator!= (const planner &o) const;
~planner ();
// Public class utilities
int erase ();
int reinitialize (int64_t base_time, uint64_t duration);
int restore_track_points ();
int update_total (uint64_t resource_total);

// Resources and duration
int64_t get_total_resources () const;
Expand Down Expand Up @@ -108,8 +113,6 @@ class planner {
// Private class utilities
int copy_trees (const planner &o);
int copy_maps (const planner &o);
bool scheduled_points_equal (const scheduled_point_t &lhs,
const scheduled_point_t &rhs) const;
bool span_lookups_equal (const planner &o) const;
bool avail_time_iters_equal (const planner &o) const;
bool trees_equal (const planner &o) const;
Expand Down
49 changes: 49 additions & 0 deletions resource/planner/c++/planner_internal_tree.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*****************************************************************************\
* Copyright 2024 Lawrence Livermore National Security, LLC
* (c.f. AUTHORS, NOTICE.LLNS, LICENSE)
*
* This file is part of the Flux resource manager framework.
* For details, see https://github.com/flux-framework.
*
* SPDX-License-Identifier: LGPL-3.0
\*****************************************************************************/

extern "C" {
#if HAVE_CONFIG_H
#include "config.h"
#endif
}

#include "planner_internal_tree.hpp"

bool scheduled_point_t::operator== (const scheduled_point_t &o) const
{
if (point_rb != o.point_rb)
return false;
if (resource_rb != o.resource_rb)
return false;
if (at != o.at)
return false;
if (in_mt_resource_tree != o.in_mt_resource_tree)
return false;
if (new_point != o.new_point)
return false;
if (ref_count != o.ref_count)
return false;
if (remaining != o.remaining)
return false;
if (scheduled != o.scheduled)
return false;

return true;
}

bool scheduled_point_t::operator!= (const scheduled_point_t &o) const
{
return !operator == (o);
}


/*
* vi: ts=4 sw=4 expandtab
*/
3 changes: 3 additions & 0 deletions resource/planner/c++/planner_internal_tree.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
* tree.
*/
struct scheduled_point_t {
bool operator== (const scheduled_point_t &o) const;
bool operator!= (const scheduled_point_t &o) const;

scheduled_point_rb_node_t point_rb; /* BST node for scheduled point tree */
mt_resource_rb_node_t resource_rb; /* BST node for min-time resource tree */
int64_t at; /* Resource-state changing time */
Expand Down
Loading
Loading