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.

Sign up for GitHub

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

Jump to bottom

Performance and safety improvements #429

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Performance and safety improvements #429

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4761ab1
Replaced regex with custom string parsing and a few more low hanging …
mikucionisaau Oct 23, 2024
b5906e2
Fixed from_chars calls
mikucionisaau Oct 23, 2024
1f5d92c
Fixed some code readability
mikucionisaau Oct 24, 2024
29460e0
Moved the version-dependent checks into gnuplot scope
mikucionisaau Oct 24, 2024
86b1dc7
Removed struct version in favor of tuple for the sake of old API
mikucionisaau Oct 24, 2024
c7e74d5
Formatting
mikucionisaau Oct 24, 2024
93a0ff5
Fixed readability when converting string_view to int
mikucionisaau Oct 24, 2024
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
27 changes: 26 additions & 1 deletion source/matplot/backend/backend_interface.h
View file
Open in desktop
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,31 @@ 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

/// Captures (backend) version information.
struct Version {
Copy link
Owner

Choose a reason for hiding this comment

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

This needs MATPLOT_EXPORTS if it's part of the API.

Copy link
Owner

Choose a reason for hiding this comment

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

It would also be good to maintain the snake_case convention. version or maybe gnuplot::version since there's no other backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make it snake case, sorry.
I could also move Version to gnuplot, I just thought the class is more general than just gnuplot.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes. Outside gnuplot is also OK. I was thinking about the name-grabbing, but that's also OK. We won't need matplot::version for anything else.

int major{0}, minor{0}, patch{0};
constexpr bool operator==(const Version &o) const {
return major == o.major && minor == o.minor && patch == o.patch;
}
constexpr bool operator!=(const Version &other) const { return !(*this == other); }
constexpr bool operator<(const Version &other) const {
if (major < other.major)
Copy link
Owner

Choose a reason for hiding this comment

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

Is it the same as std::tie(major, minor, patch) < std::tie(other.major, other.minor, other.patch)?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes. Looking at the rest of the code and considering tuple defines operator<, I do prefer Version over tuple here if we were coming up with the original API now, but I don't think breaking it now brings a lot of value compared to the potential hassle of people opening issues because the API changed.

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 is gone, sorry my bad.

return true;
else if (major == other.major) {
if (minor < other.minor)
return true;
else if (minor == other.minor)
return patch < other.patch;
}
return false;
}
constexpr bool operator>(const Version &other) const { return other < *this; }
constexpr bool operator<=(const Version &other) const { return !(other < *this); }
constexpr bool operator>=(const Version &other) const { return !(other > *this); }
constexpr operator bool() const { return *this != Version{0,0,0}; }
}; // struct Version
} // namespace backend

} // namespace matplot
Expand Down
75 changes: 32 additions & 43 deletions source/matplot/backend/gnuplot.cpp
View file
Open in desktop
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,50 +349,24 @@ 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();
}

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});
Version gnuplot::gnuplot_version() {
static auto version = Version{};
if (!version) { // unknown version
const auto version_str = run_and_get_output("gnuplot --version 2>&1");
const auto major = word_after(version_str, "gnuplot");
const auto minor = word_after(major, ".");
Copy link
Owner

Choose a reason for hiding this comment

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

This pattern is a little confusing when the function is called repeatedly. Let's say the output is "... gnuplot 5.2 patchlevel 6". Wouldn't major just be "5"? If so, then there's no word after "." because major doesn't include everything after 5. That is, major is "5" and not "5.2 patchlevel 6". On the other hand, if major does contain "5.2 patchlevel 6", then the function name and return value are a little misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the name major is misleading, I have renamed it to major_minor and added example in comments -- I think it's clearer when an example is upfront.

const auto patch = word_after(version_str, "patchlevel");
if (!major.empty() && !minor.empty() && !patch.empty()) {
std::from_chars(major.data(), major.data()+major.length(), version.major);
std::from_chars(minor.data(), minor.data()+minor.length(), version.minor);
std::from_chars(patch.data(), patch.data()+patch.length(), version.patch);
}
if (!version) // still unknown
version = {5, 2, 6}; // assume by convention
}
return version;
}
Expand Down
2 changes: 1 addition & 1 deletion source/matplot/backend/gnuplot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ namespace matplot::backend {
/// Identify the default terminal type in the system
static std::string default_terminal_type();
static bool terminal_is_available(std::string_view);
static std::tuple<int, int, int> gnuplot_version();
static Version gnuplot_version();
Copy link
Owner

Choose a reason for hiding this comment

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

So converting the output to a tuple wouldn't break the API? A Version is a better decision than a tuple, but it's hard to tell how many people are counting on this API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I have reverted Version to std::tuple as I did not know that one can compare them in such elegant way. I used to kill it if the code is not about generic programming (Scott Meyers book).
I learned something today, thanks!

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
22 changes: 10 additions & 12 deletions source/matplot/core/axes_type.cpp
View file
Open in desktop
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 •
edited
Loading

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,8 +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}) {
if (backend::gnuplot::gnuplot_version() < backend::Version{5, 2, 6}) {
ok = false;
}
}
Expand Down Expand Up @@ -916,9 +915,8 @@ 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}) {
const auto v = backend::gnuplot::gnuplot_version();
if (v < backend::Version{5, 2, 6}) {
if (!msg_shown_once) {
std::cerr
<< "You need gnuplot 5.2.6+ to include legends"
Expand Down Expand Up @@ -2745,9 +2743,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 +2762,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
View file
Open in desktop
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
5 changes: 2 additions & 3 deletions source/matplot/core/figure_type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,7 @@ namespace matplot {
// 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 = (v >= backend::Version{5,5,0});
// 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 +894,4 @@ namespace matplot {

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

} // namespace matplot
} // namespace matplot