Performance and safety improvements #429

mikucionisaau · 2024-10-23T12:37:00Z

replace std::regex with std:string_view scanning (thousands of times faster)
replace std::tuple with dedicated Version class with named members
add virtual destructor to backend_interface for safety
replaced value passing with const & in fplot and more
clang_tidy suggests making lots of const methods and more...

…improvements

mikucionisaau · 2024-10-24T07:59:02Z

Rebased and tested with GCC/Debian/testing, Windows Visual Studio Community 2022 and AppleClang/MacOS.
Feels faster and snappier.
The rest of clang-tidy suggestions are too minor and too numerous.

Potential reservations: changes the API subtly, not sure about consequences for other projects which use this.
I think this is ready and worth merging.

alandefreitas

Nice. Removing std::regex is refreshing. :)

alandefreitas · 2024-10-24T14:03:38Z

source/matplot/backend/backend_interface.h

+        }; // class backend_interface
+
+        /// Captures (backend) version information.
+        struct Version {


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

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

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.

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.

alandefreitas · 2024-10-24T14:05:18Z

source/matplot/backend/gnuplot.cpp

            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 ");


Heh... Great! :)

alandefreitas · 2024-10-24T14:10:07Z

source/matplot/backend/gnuplot.cpp

+        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, ".");


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.

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.

alandefreitas · 2024-10-24T14:11:46Z

source/matplot/backend/gnuplot.h

        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();


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.

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!

alandefreitas · 2024-10-24T14:13:53Z

source/matplot/core/axes_type.cpp

        }
        auto set_or_unset_axis = [this](class axis_type &ax,
-                                        std::string axis_name,
+                                        const std::string &axis_name,


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.

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.

alandefreitas · 2024-10-24T14:15:04Z

source/matplot/backend/backend_interface.h

+            }
+            constexpr bool operator!=(const Version &other) const { return !(*this == other); }
+            constexpr bool operator<(const Version &other) const {
+                if (major < other.major)


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

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.

This is gone, sorry my bad.

alandefreitas · 2024-10-24T14:18:10Z

source/matplot/core/axes_type.cpp

-    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,


Same here. All these API changes are OK.

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

mikucionisaau · 2024-10-24T17:28:23Z

I have removed my version mess as tuple is simpler (it aleady has all operators) and thus API is preserved.
I moved the code concerned with versioning into gnuplot class, which makes the code cleaner as the feature knowledge is maintained in one place.

mikucionisaau · 2024-10-24T17:33:21Z

source/matplot/core/figure_type.cpp

-        const bool has_wall_option =
-            std::get<0>(v) > 5 || (std::get<0>(v) == 5 && std::get<1>(v) >= 5);


this was my trigger :-)

Please-just-dont · 2025-09-19T01:58:23Z

I noticed you made a change to take an argument by reference, but I've noticed that the first arguments to a lot of these functions, the first set of axes is always taken by copy, and I'm wondering why it's not taken by const reference.

mikucionisaau · 2025-09-20T12:08:36Z

I noticed you made a change to take an argument by reference, but I've noticed that the first arguments to a lot of these functions, the first set of axes is always taken by copy, and I'm wondering why it's not taken by const reference.

@Please-just-dont Could you please be more specific? Which code lines exactly?
It was not my original intention to change to references, but those came into my way I tried to improve the performance. The code probably needs to be revised more thoroughly.

alandefreitas · 2025-09-21T20:15:54Z

How's this PR going? I don't remember if anything is blocking it since the last review.

It's a somewhat unrelated issue, but my intuition is that this would break CI (not because it's bad, but simply because big changes often break it), while at the same time, ci.yml must be broken because it hasn't been maintained in a while.

mikucionisaau · 2025-09-22T08:27:43Z

@alandefreitas I think this PR is ready for merge, unless there are more comments/requests. I see that ci.yml uses lots of custom actions -- that's just too much for me to stomach at this point, but I'd be interested in what comes out.

mikucionisaau · 2025-09-22T09:31:35Z

I just realized that my build directory is of 21G in size, so that's a big effort just to compile and testing is complicated by the wait (ctest does not allow interaction).
So briefly: I've got it all built with -DMATPLOTPP_BUILD_EXAMPLES=ON -DMATPLOTPP_BUILD_TESTS=ON and tested using GNUTERM=dumb ninja test and holding ENTER key just to complete all 545 tests, and it all passed.

Let me know if there is anything more I can do to test at my end, before dispatching this massive test on CI.

alandefreitas · 2025-09-22T15:30:10Z

Yes. Everything you mention is OK. I just mean regular CI is not running. Our tests aren't that great but at least we got to see if everything was compiling before. Anyway, this should be OK. Let's not block it because of an unrelated issue.

mikucionisaau force-pushed the performance branch from ef76547 to 69591ea Compare October 23, 2024 12:41

mikucionisaau added 2 commits October 24, 2024 09:11

Replaced regex with custom string parsing and a few more low hanging …

4761ab1

…improvements

Fixed from_chars calls

b5906e2

mikucionisaau force-pushed the performance branch from d3a418b to b5906e2 Compare October 24, 2024 07:16

Fixed some code readability

1f5d92c

mikucionisaau marked this pull request as ready for review October 24, 2024 07:56

alandefreitas reviewed Oct 24, 2024

View reviewed changes

mikucionisaau added 3 commits October 24, 2024 18:44

Moved the version-dependent checks into gnuplot scope

29460e0

Removed struct version in favor of tuple for the sake of old API

86b1dc7

Formatting

c7e74d5

mikucionisaau requested a review from alandefreitas October 24, 2024 17:25

mikucionisaau commented Oct 24, 2024

View reviewed changes

Fixed readability when converting string_view to int

93a0ff5

alandefreitas merged commit dc4efce into alandefreitas:master Sep 22, 2025

mikucionisaau deleted the performance branch September 22, 2025 16:45

		const bool has_wall_option =
		std::get<0>(v) > 5 \|\| (std::get<0>(v) == 5 && std::get<1>(v) >= 5);

Performance and safety improvements #429

Performance and safety improvements #429

Conversation

mikucionisaau commented Oct 23, 2024 • edited by alandefreitas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikucionisaau commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alandefreitas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikucionisaau Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikucionisaau commented Oct 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Please-just-dont commented Sep 19, 2025

Uh oh!

mikucionisaau commented Sep 20, 2025

Uh oh!

alandefreitas commented Sep 21, 2025

Uh oh!

mikucionisaau commented Sep 22, 2025

Uh oh!

mikucionisaau commented Sep 22, 2025

Uh oh!

alandefreitas commented Sep 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikucionisaau commented Oct 23, 2024 •

edited by alandefreitas

Loading

mikucionisaau commented Oct 24, 2024 •

edited

Loading

mikucionisaau Oct 24, 2024 •

edited

Loading