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

Performance and safety improvements #429

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
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
3 changes: 2 additions & 1 deletion source/matplot/backend/backend_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ namespace matplot {
class MATPLOT_EXPORTS backend_interface {
/// Virtual functions you can override to create any backend
public:
virtual ~backend_interface() noexcept = default;
/// \brief True if backend is in interactive mode
/// One backends might support both interactive and
/// non-interactive mode.
Expand Down Expand Up @@ -213,7 +214,7 @@ namespace matplot {
/// This is useful when tracing the gnuplot commands
/// and when generating a gnuplot file.
virtual void include_comment(const std::string &text);
};
}; // class backend_interface
} // namespace backend

} // namespace matplot
Expand Down
90 changes: 48 additions & 42 deletions source/matplot/backend/gnuplot.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include <matplot/util/common.h>
#include <matplot/util/popen.h>
#include <iostream>
#include <regex>
#include <charconv>
#include <thread>
#include <cstring>
#include <cstdlib>
Expand Down Expand Up @@ -317,13 +317,28 @@ namespace matplot::backend {
}
}

/// returns the next word in text after prefix terminated with white space.
static std::string_view word_after(std::string_view text, std::string_view prefix)
{
auto res = text.substr(0,0);
if (auto b = text.find(prefix); b != std::string_view::npos) {
b += prefix.length();
while (b < text.length() && std::isspace(text[b]))
++b; // skip white space before word
auto e = b;
while (e < text.length() && !std::isspace(text[e]))
++e; // scan until white space or end
res = text.substr(b, e-b);
}
return res;
}

std::string gnuplot::default_terminal_type() {
static std::string terminal_type;
const bool dont_know_term_type = terminal_type.empty();
if (dont_know_term_type) {
terminal_type = run_and_get_output("gnuplot -e \"show terminal\" 2>&1");
terminal_type = std::regex_replace(terminal_type,
std::regex("[^]*terminal type is ([^ ]+)[^]*"), "$1");
terminal_type = word_after(terminal_type, "terminal type is ");
Copy link
Owner

Choose a reason for hiding this comment

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

Heh... Great! :)

const bool still_dont_know_term_type = terminal_type.empty();
if (still_dont_know_term_type) {
terminal_type = "qt";
Expand All @@ -334,54 +349,45 @@ namespace matplot::backend {

bool gnuplot::terminal_is_available(std::string_view term) {
std::string msg = run_and_get_output("gnuplot -e \"set terminal " +
std::string(term.data()) + "\" 2>&1");
std::string{term} + "\" 2>&1");
return msg.empty();
}

template <typename T>
void convert_to(std::string_view text, T& value) {
std::from_chars(text.data(), text.data() + text.length(), value);
}

std::tuple<int, int, int> gnuplot::gnuplot_version() {
static std::tuple<int, int, int> version{0, 0, 0};
const bool dont_know_gnuplot_version_yet =
version == std::tuple<int, int, int>({0, 0, 0});
if (dont_know_gnuplot_version_yet) {
std::string version_str =
run_and_get_output("gnuplot --version 2>&1");
std::string version_major = std::regex_replace(
version_str,
std::regex("[^]*gnuplot (\\d+)\\.\\d+ patchlevel \\d+ *"),
"$1");
std::string version_minor = std::regex_replace(
version_str,
std::regex("[^]*gnuplot \\d+\\.(\\d+) patchlevel \\d+ *"),
"$1");
std::string version_patch = std::regex_replace(
version_str,
std::regex("[^]*gnuplot \\d+\\.\\d+ patchlevel (\\d+) *"),
"$1");
try {
std::get<0>(version) = std::stoi(version_major);
} catch (...) {
std::get<0>(version) = 0;
}
try {
std::get<1>(version) = std::stoi(version_minor);
} catch (...) {
std::get<1>(version) = 0;
}
try {
std::get<2>(version) = std::stoi(version_patch);
} catch (...) {
std::get<2>(version) = 0;
}
const bool still_dont_know_gnuplot_version =
version == std::tuple<int, int, int>({0, 0, 0});
if (still_dont_know_gnuplot_version) {
// assume it's 5.2.6 by convention
version = std::tuple<int, int, int>({5, 2, 6});
constexpr auto version_zero = std::make_tuple(0, 0, 0);
static auto version = version_zero;
if (version == version_zero) { // unknown version
const auto version_str = run_and_get_output("gnuplot --version 2>&1");
// gnuplot version_str example: "5.2 patchlevel 6"
const auto major_minor = word_after(version_str, "gnuplot"); // "5.2"
const auto minor = word_after(major_minor, "."); // "2"
const auto patch = word_after(version_str, "patchlevel"); // "6"
if (!major_minor.empty() && !minor.empty() && !patch.empty()) {
convert_to(major_minor, std::get<0>(version));
convert_to(minor, std::get<1>(version));
convert_to(patch, std::get<2>(version));
}
if (version == version_zero) // still unknown
version = {5, 2, 6}; // assume by convention
}
return version;
}

bool gnuplot::gnuplot_includes_legends() {
return gnuplot_version() >= std::make_tuple(5, 2, 6);
}
bool gnuplot::gnuplot_has_wall_option() {
return gnuplot_version() >= std::make_tuple(5, 5, 0);
}
bool gnuplot::gnuplot_supports_keyentry() {
return gnuplot_version() >= std::make_tuple(5, 2, 6);
}

bool gnuplot::terminal_has_title_option(const std::string &t) {
SV_CONSTEXPR std::string_view whitelist[] = {
"qt", "aqua", "caca", "canvas", "windows", "wxt", "x11"};
Expand Down
3 changes: 3 additions & 0 deletions source/matplot/backend/gnuplot.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ namespace matplot::backend {
static std::string default_terminal_type();
static bool terminal_is_available(std::string_view);
static std::tuple<int, int, int> gnuplot_version();
static bool gnuplot_includes_legends();
static bool gnuplot_has_wall_option();
static bool gnuplot_supports_keyentry();
static bool terminal_has_title_option(const std::string &t);
static bool terminal_has_size_option(const std::string &t);
static bool terminal_has_position_option(const std::string &t);
Expand Down
23 changes: 9 additions & 14 deletions source/matplot/core/axes_type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ namespace matplot {
run_command("set polar");
}
auto set_or_unset_axis = [this](class axis_type &ax,
std::string axis_name,
const std::string &axis_name,
Copy link
Owner

Choose a reason for hiding this comment

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

If that was your concern about breaking the API, both std::string axis_name and std::string const& axis_name are OK. In the implementation the std::string axis_name can be moved and the std::string const& axis_name can be copied. As far as the user is concerned, that doesn't functionally break the API. Only std::string &axis_name would be a problem.

Copy link
Contributor Author

@mikucionisaau mikucionisaau Oct 24, 2024

Choose a reason for hiding this comment

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

No concerns here: that is just a lambda expression - internal stuff.
And yes, it'll bind to the same calls as previously, we just need to make sure that the lambda expression itself does not modify the parameters, but that is checked by the compiler.

bool minor_ticks = false) {
// cb is the only axis we don't unset if tics are empty
// r-axis labels should still be handled even if axis is invisible since we use the grid
Expand Down Expand Up @@ -694,10 +694,7 @@ namespace matplot {
// Gnuplot version needs to be 5.2.6+ for keyentry
bool ok = true;
if (parent_->backend_->consumes_gnuplot_commands()) {
if (backend::gnuplot::gnuplot_version() <
std::tuple<int, int, int>{5, 2, 6}) {
ok = false;
}
ok = backend::gnuplot::gnuplot_supports_keyentry();
}
if (legend_ == nullptr || !legend_->visible() || !ok) {
run_command("set key off");
Expand Down Expand Up @@ -916,9 +913,7 @@ namespace matplot {
static bool msg_shown_once = false;
// Gnuplot version needs to be 5.2.6+ for keyentry
if (parent_->backend_->consumes_gnuplot_commands()) {
std::tuple<int, int, int> v =
backend::gnuplot::gnuplot_version();
if (v < std::tuple<int, int, int>{5, 2, 6}) {
if (backend::gnuplot::gnuplot_includes_legends()) {
if (!msg_shown_once) {
std::cerr
<< "You need gnuplot 5.2.6+ to include legends"
Expand Down Expand Up @@ -2745,9 +2740,9 @@ namespace matplot {
}

std::vector<function_line_handle>
axes_type::fplot(std::vector<function_line::function_type> equations,
std::array<double, 2> x_range,
std::vector<std::string> line_specs) {
axes_type::fplot(const std::vector<function_line::function_type> &equations,
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. All these API changes are OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's OK if the thing is recompiled, but I am not sure at ABI level, and I do not know if anyone cares.

const std::array<double, 2> &x_range,
const std::vector<std::string> &line_specs) {
axes_silencer temp_silencer_{this};
std::vector<function_line_handle> res;
auto it_line_specs = line_specs.begin();
Expand All @@ -2764,9 +2759,9 @@ namespace matplot {
}

std::vector<function_line_handle>
axes_type::fplot(std::vector<function_line::function_type> equations,
std::vector<double> x_range,
std::vector<std::string> line_specs) {
axes_type::fplot(const std::vector<function_line::function_type>& equations,
const std::vector<double>& x_range,
const std::vector<std::string>& line_specs) {
return this->fplot(equations, to_array<2>(x_range), line_specs);
}

Expand Down
12 changes: 6 additions & 6 deletions source/matplot/core/axes_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -979,15 +979,15 @@ namespace matplot {

/// Lambda function line plot - list of functions
std::vector<function_line_handle>
fplot(std::vector<function_line::function_type> equations,
std::array<double, 2> x_range = {-5, 5},
std::vector<std::string> line_specs = {});
fplot(const std::vector<function_line::function_type> &equations,
const std::array<double, 2> &x_range = {-5, 5},
const std::vector<std::string> &line_specs = {});

/// Lambda function line plot - list of functions and line specs
std::vector<function_line_handle>
fplot(std::vector<function_line::function_type> equations,
std::vector<double> x_range,
std::vector<std::string> line_specs = {});
fplot(const std::vector<function_line::function_type> &equations,
const std::vector<double> &x_range,
const std::vector<std::string> &line_specs = {});

using implicit_function_type = std::function<double(double, double)>;

Expand Down
6 changes: 2 additions & 4 deletions source/matplot/core/figure_type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -633,9 +633,7 @@ namespace matplot {
void figure_type::run_window_color_command() {
// In gnuplot 5.5 we have the wall function to set the axes color
// with a rectangle workaround, which does not work well for 3d.
static const auto v = backend::gnuplot::gnuplot_version();
const bool has_wall_option =
std::get<0>(v) > 5 || (std::get<0>(v) == 5 && std::get<1>(v) >= 5);
Comment on lines -637 to -638
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was my trigger :-)

const bool has_wall_option = backend::gnuplot::gnuplot_has_wall_option();
// So we only plot the default background if it's not 3d or version is
// higher than 5.5. Otherwise, gnuplot won't be able to set the axes
// colors.
Expand Down Expand Up @@ -895,4 +893,4 @@ namespace matplot {

bool figure_type::should_close() { return backend_->should_close(); }

} // namespace matplot
} // namespace matplot