Add the statistic function in the profiler #376

ThreeMonth03 · 2024-06-24T13:10:47Z

According to the pr 343,
I refer the cprofiler(https://docs.python.org/3/library/profile.html),
and add the statistic function in the callprofiler.

ThreeMonth03

@yungyuc
Here is the pr of the issue 343,
but it is not ready for code review.

ThreeMonth03 · 2024-06-24T13:15:56Z

cpp/modmesh/toggle/profile.hpp

    std::string report() const
    {
        std::ostringstream ostm;
+        ostm << "      " << total_call_count() << " function calls in " << total_time() << " seconds" << std::endl;
+        ostm << std::endl;
+        ostm << std::setw(40) << "Function Name" << std::setw(25) << "Call Count" << std::setw(25) << "Total Time (s)" << std::setw(25) << "Per Call (s)" << std::setw(25) << "Cumulative Time (s)" << std::setw(25) << "Per Call (s)" << std::endl;
        for (auto it = m_entry.begin(); it != m_entry.end(); ++it)
        {
-            ostm
-                << it->first << " : "
-                << "count = " << it->second.count() << " , "
-                << "time = " << it->second.time() << " (second)"
-                << std::endl;
+            ostm << std::setw(40) << it->first << std::setw(25) << it->second.count() << std::setw(25) << it->second.time() << std::setw(25) << it->second.time() / it->second.count() << std::setw(25) << it->second.ctime() << std::setw(25) << it->second.ctime() / it->second.count() << std::endl;
        }
        return ostm.str();
    }


It seems that enhancement of the report function make the CI fail.
I will fix the modification later.

ThreeMonth03

@yungyuc I'm ready for reviewing the code.

ThreeMonth03 · 2024-06-24T17:13:30Z

cpp/modmesh/toggle/RadixTree.cpp

+void CallProfiler::print_statistics(const RadixTreeNode<CallerProfile> & node, std::ostream & outstream)
+{
+    TimeRegistry::me().clear();
+
+    std::queue<const RadixTreeNode<CallerProfile> *> nodes_buffer;
+    for (const auto & child : node.children())
+    {
+        nodes_buffer.push(child.get());
+    }
+
+    // BFS algorithm and put the node information into TimeRegistry.
+    while (!nodes_buffer.empty())
+    {
+        const int nodes_buffer_size = nodes_buffer.size();
+        for (int i = 0; i < nodes_buffer_size; ++i)
+        {
+            const RadixTreeNode<CallerProfile> * current_node = nodes_buffer.front();
+            nodes_buffer.pop();
+            TimeRegistry::me().add(current_node->data().caller_name, current_node->data().total_time.count() / 1e9, current_node->data().total_time.count() / 1e9, current_node->data().call_count);
+
+            for (const auto & child : current_node->children())
+            {
+                nodes_buffer.push(child.get());
+                TimeRegistry::me().add(current_node->data().caller_name, 0, -child->data().total_time.count() / 1e9, 0);
+            }
+        }
+    }
+
+    // Print the statistics.
+    outstream << TimeRegistry::me().detailed_report() << std::endl;
+
+    // Reset the TimeRegistry.
+    TimeRegistry::me().clear();
+}


I orginize the data, and write into TimeRegistry.
Since TimeRegistry is singleton pattern,
I'm not sure whether the modification of TimeRegistrymay affect the other function.

ThreeMonth03 · 2024-06-24T17:14:56Z

cpp/modmesh/toggle/profile.hpp

+    std::string detailed_report() const
+    {
+        std::ostringstream ostm;
+        /// Header
+        ostm
+            << "      " << total_call_count()
+            << " function calls in " << total_time()
+            << " seconds" << std::endl;
+        ostm << std::endl;
+        ostm
+            << std::setw(40) << "Function Name"
+            << std::setw(25) << "Call Count"
+            << std::setw(25) << "Total Time (s)"
+            << std::setw(25) << "Per Call (s)"
+            << std::setw(25) << "Cumulative Time (s)"
+            << std::setw(25) << "Per Call (s)"
+            << std::endl;
+
+        /// Body
+        for (auto it = m_entry.begin(); it != m_entry.end(); ++it)
+        {
+            ostm
+                << std::setw(40) << it->first
+                << std::setw(25) << it->second.count()
+                << std::setw(25) << it->second.time()
+                << std::setw(25) << it->second.time() / it->second.count()
+                << std::setw(25) << it->second.ctime()
+                << std::setw(25) << it->second.ctime() / it->second.count()
+                << std::endl;
+        }
+        return ostm.str();
+    }


I add a new function to make the report clearly.
Additionally, by remaining the old feature std::string report() const,
I think we could prevent from rewriting the pytest.

yungyuc

Be mindful to coding styles.

Properly add blank lines.
Use Python tests for printed results, not C++.
Avoid output arguments.

yungyuc · 2024-06-25T09:38:25Z

cpp/modmesh/toggle/RadixTree.cpp

@@ -27,7 +27,7 @@
 */

 #include <modmesh/toggle/RadixTree.hpp>
-
+#include <modmesh/toggle/profile.hpp>


Don't forget to keep a blank line between include and the following constructs.

yungyuc · 2024-06-25T09:40:12Z

gtests/test_nopython_callprofiler.cpp

@@ -212,5 +212,124 @@ TEST_F(CallProfilerTest, cancel)
    EXPECT_EQ(radix_tree().get_unique_node(), 0);
 }

+std::string line2 = "";


This kind of tests, which assert for printed texts, should be in Python, not in C++.

yungyuc · 2024-06-25T09:42:04Z

cpp/modmesh/toggle/RadixTree.hpp

@@ -291,6 +297,7 @@ class CallProfiler

 private:
    void print_profiling_result(const RadixTreeNode<CallerProfile> & node, const int depth, std::ostream & outstream) const;
+    static void print_statistics(const RadixTreeNode<CallerProfile> & node, std::ostream & outstream);


Output arguments are hard to maintain. Please avoid output arguments.

@yungyuc for std::ostream, isn't it a common pattern to use as an output argument?
We can return a string directly, but I think using a stream is more efficient. In addition, we have flexibility to put std::cout or std::stringstream here.

I am not sure if you notice it or not, we also have void print_profiling_result(const RadixTreeNode<CallerProfile> & node, const int depth, std::ostream & outstream) const; already.

@yungyuc for std::ostream, isn't it a common pattern to use as an output argument? We can return a string directly, but I think using a stream is more efficient. In addition, we have flexibility to put std::cout or std::stringstream here.

It is common to use ostream as an output argument, but it is better to do it in an operator>>() overload.

I can tolerate output arguments when they are with a private function. It turned out that I did not notice that the helpers are private member functions. Then I am fine with it. But not because std::ostream.

ThreeMonth03

Properly add blank lines.
Use Python tests for printed results, not C++.

@yungyuc I'm ready for next review.

ThreeMonth03 · 2024-07-01T11:19:17Z

cpp/modmesh/toggle/RadixTree.cpp

@@ -27,7 +27,7 @@
 */

 #include <modmesh/toggle/RadixTree.hpp>
-
+#include <modmesh/toggle/profile.hpp>


ThreeMonth03 · 2024-07-01T11:20:42Z

tests/test_callprofiler.py

-        path = os.path.join(os.path.abspath(os.path.dirname(__file__)),
-                            "data", "profiler_python_schema.json")
-        with open(path, 'r') as schema_file:
+        path = os.path.join(
+            os.path.abspath(os.path.dirname(__file__)),
+            "data",
+            "profiler_python_schema.json",
+        )
+        with open(path, "r") as schema_file:


I format the file by vscode extension.
I don't rewrite the code.

Please revert the style change. Do not mix style change with logic change in one PR.

A style change is a refactoring change and should use its own PR.

ThreeMonth03 · 2024-07-01T11:21:14Z

tests/test_callprofiler.py

+
+    def test_get_stat(self):
+        time1 = 0.5
+        time2 = 0.1
+        time3 = 0.2
+
+        @profile_function
+        def bar():
+            busy_loop(time1)
+
+        @profile_function
+        def foo():
+            busy_loop(time2)
+            bar()
+
+        @profile_function
+        def baz():
+            busy_loop(time3)
+            foo()
+
+        modmesh.call_profiler.reset()
+        bar()
+        bar()
+        foo()
+        baz()
+        root_stat = modmesh.call_profiler.stat()
+
+        # Check the number of lines
+        stat_line_list = root_stat.split("\n")
+        self.assertEqual(len(stat_line_list), 7)
+
+        # Check the first line
+        words = stat_line_list[0].split()
+        self.assertEqual(words[0], "7")
+        self.assertEqual(words[1], "function")
+        self.assertEqual(words[2], "calls")
+        self.assertEqual(words[3], "in")
+        ref_total_time = time1 * 4 + time2 * 2 + time3
+        self.assertTrue(abs(float(words[4]) - ref_total_time) <= 6e-4)
+        self.assertEqual(words[5], "seconds")
+
+        # Check the second line
+        self.assertEqual(stat_line_list[1], "")
+
+        # Check the third line
+        ref_line3 = (
+            "                           Function Name"
+            + "               Call Count"
+            + "           Total Time (s)"
+            + "             Per Call (s)"
+            + "      Cumulative Time (s)"
+            + "             Per Call (s)"
+        )
+        self.assertEqual(stat_line_list[2], ref_line3)
+
+        # Check remaining lines
+        stat_dict = {}
+        for line in stat_line_list[3:-1]:
+            words = line.split()
+            stat_dict[words[0]] = {
+                "call_count": int(words[1]),
+                "total_time": float(words[2]),
+                "total_per_call": float(words[3]),
+                "cumulative_time": float(words[4]),
+                "cumulative_per_call": float(words[5]),
+            }
+        bar_dict = stat_dict["bar"]
+        self.assertEqual(bar_dict["call_count"], 4)
+        self.assertTrue(bar_dict["total_time"] - (time1 * 4) <= 3e-4)
+        self.assertTrue(bar_dict["total_per_call"] - time1 <= 3e-4)
+        self.assertTrue(bar_dict["cumulative_time"] - (time1 * 4) <= 3e-4)
+        self.assertTrue(bar_dict["cumulative_per_call"] - time1 <= 3e-4)
+
+        foo_dict = stat_dict["foo"]
+        self.assertEqual(foo_dict["call_count"], 2)
+        self.assertTrue(foo_dict["total_time"] - (time1 + time2) * 2 <= 3e-4)
+        self.assertTrue(foo_dict["total_per_call"] - (time1 + time2) <= 3e-4)
+        self.assertTrue(foo_dict["cumulative_time"] - (time2 * 2) <= 3e-4)
+        self.assertTrue(foo_dict["cumulative_per_call"] - time2 <= 3e-4)
+
+        baz_dict = stat_dict["baz"]
+        ref_total_time = time1 + time2 + time3
+        self.assertEqual(baz_dict["call_count"], 1)
+        self.assertTrue(baz_dict["total_time"] - ref_total_time <= 3e-4)
+        self.assertTrue(baz_dict["total_per_call"] - ref_total_time <= 3e-4)
+        self.assertTrue(baz_dict["cumulative_time"] - time3 <= 3e-4)
+        self.assertTrue(baz_dict["cumulative_per_call"] - time3 <= 3e-4)


I move the gtest to pytest.

It now looks nicer and easy to understand. Thanks.

yungyuc

Revert unnecessary changes like style (non-logical) to avoid confusion.
Rebase and squash.
Use assertLessEqual() and alikes.
Fix indentation.
Take the long lambda for CallProfiler.result() to a static member function in WrapCallProfiler.

yungyuc · 2024-07-03T04:39:34Z

tests/test_callprofiler.py

-        path = os.path.join(os.path.abspath(os.path.dirname(__file__)),
-                            "data", "profiler_python_schema.json")
-        with open(path, 'r') as schema_file:
+        path = os.path.join(
+            os.path.abspath(os.path.dirname(__file__)),
+            "data",
+            "profiler_python_schema.json",
+        )
+        with open(path, "r") as schema_file:


Please revert the style change. Do not mix style change with logic change in one PR.

A style change is a refactoring change and should use its own PR.

yungyuc · 2024-07-03T04:42:31Z

tests/test_callprofiler.py

+        self.assertEqual(words[2], "calls")
+        self.assertEqual(words[3], "in")
+        ref_total_time = time1 * 4 + time2 * 2 + time3
+        self.assertTrue(abs(float(words[4]) - ref_total_time) <= 6e-4)


Use assertLessEqual() and alikes.

yungyuc · 2024-07-03T04:43:16Z

tests/test_callprofiler.py

+
+    def test_get_stat(self):
+        time1 = 0.5
+        time2 = 0.1
+        time3 = 0.2
+
+        @profile_function
+        def bar():
+            busy_loop(time1)
+
+        @profile_function
+        def foo():
+            busy_loop(time2)
+            bar()
+
+        @profile_function
+        def baz():
+            busy_loop(time3)
+            foo()
+
+        modmesh.call_profiler.reset()
+        bar()
+        bar()
+        foo()
+        baz()
+        root_stat = modmesh.call_profiler.stat()
+
+        # Check the number of lines
+        stat_line_list = root_stat.split("\n")
+        self.assertEqual(len(stat_line_list), 7)
+
+        # Check the first line
+        words = stat_line_list[0].split()
+        self.assertEqual(words[0], "7")
+        self.assertEqual(words[1], "function")
+        self.assertEqual(words[2], "calls")
+        self.assertEqual(words[3], "in")
+        ref_total_time = time1 * 4 + time2 * 2 + time3
+        self.assertTrue(abs(float(words[4]) - ref_total_time) <= 6e-4)
+        self.assertEqual(words[5], "seconds")
+
+        # Check the second line
+        self.assertEqual(stat_line_list[1], "")
+
+        # Check the third line
+        ref_line3 = (
+            "                           Function Name"
+            + "               Call Count"
+            + "           Total Time (s)"
+            + "             Per Call (s)"
+            + "      Cumulative Time (s)"
+            + "             Per Call (s)"
+        )
+        self.assertEqual(stat_line_list[2], ref_line3)
+
+        # Check remaining lines
+        stat_dict = {}
+        for line in stat_line_list[3:-1]:
+            words = line.split()
+            stat_dict[words[0]] = {
+                "call_count": int(words[1]),
+                "total_time": float(words[2]),
+                "total_per_call": float(words[3]),
+                "cumulative_time": float(words[4]),
+                "cumulative_per_call": float(words[5]),
+            }
+        bar_dict = stat_dict["bar"]
+        self.assertEqual(bar_dict["call_count"], 4)
+        self.assertTrue(bar_dict["total_time"] - (time1 * 4) <= 3e-4)
+        self.assertTrue(bar_dict["total_per_call"] - time1 <= 3e-4)
+        self.assertTrue(bar_dict["cumulative_time"] - (time1 * 4) <= 3e-4)
+        self.assertTrue(bar_dict["cumulative_per_call"] - time1 <= 3e-4)
+
+        foo_dict = stat_dict["foo"]
+        self.assertEqual(foo_dict["call_count"], 2)
+        self.assertTrue(foo_dict["total_time"] - (time1 + time2) * 2 <= 3e-4)
+        self.assertTrue(foo_dict["total_per_call"] - (time1 + time2) <= 3e-4)
+        self.assertTrue(foo_dict["cumulative_time"] - (time2 * 2) <= 3e-4)
+        self.assertTrue(foo_dict["cumulative_per_call"] - time2 <= 3e-4)
+
+        baz_dict = stat_dict["baz"]
+        ref_total_time = time1 + time2 + time3
+        self.assertEqual(baz_dict["call_count"], 1)
+        self.assertTrue(baz_dict["total_time"] - ref_total_time <= 3e-4)
+        self.assertTrue(baz_dict["total_per_call"] - ref_total_time <= 3e-4)
+        self.assertTrue(baz_dict["cumulative_time"] - time3 <= 3e-4)
+        self.assertTrue(baz_dict["cumulative_per_call"] - time3 <= 3e-4)


It now looks nicer and easy to understand. Thanks.

yungyuc · 2024-07-03T04:45:04Z

cpp/modmesh/toggle/pymod/wrap_profile.cpp

@@ -181,6 +181,11 @@ class MODMESH_PYTHON_WRAPPER_VISIBILITY WrapCallProfiler : public WrapBase<WrapC
                "instance",
                [](py::object const &) -> wrapped_type &
                { return wrapped_type::instance(); })
+            .def("stat", [](CallProfiler & profiler)


The indentation is off.

yungyuc · 2024-07-03T04:50:08Z

cpp/modmesh/toggle/pymod/wrap_profile.cpp

+                 {
+                std::stringstream ss;
+                profiler.print_statistics(ss);
+                return ss.str(); })
            .def("result", [](CallProfiler & profiler)
                 {
            const RadixTreeNode<CallerProfile> * root = profiler.radix_tree().get_root();


The indentation is off, but it is not a major issue needing to be fixed in this PR.

The function is too long to be reasonably placed in a lambda. Please replace the lambda with a static function in this wrapper class. You can reference the wrapping code for SimpleArray.__setitem__():

.def("__setitem__", &property_helper::setitem_parser)

But you do not need to use a separate helper class like property_helper.

ThreeMonth03

Revert unnecessary changes like style (non-logical) to avoid confusion.
Rebase and squash.
Use assertLessEqual() and alikes.
Fix indentation.
Take the long lambda for CallProfiler.result() to a static member function in WrapCallProfiler.

@yungyuc I'm ready for next review. Thank you very much.

ThreeMonth03 · 2024-07-06T19:11:19Z

cpp/modmesh/toggle/pymod/wrap_profile.cpp

+            .def(
+                "stat",
+                [](CallProfiler & profiler)
+                {
+                    std::stringstream ss;
+                    profiler.print_statistics(ss);
+                    return ss.str();
+                })
+            .def("result", &result)


I fix the wierd indent, and replace the long lambda function with member function.

Bravo! Thanks a lot.

ThreeMonth03 · 2024-07-06T19:19:49Z

tests/test_callprofiler.py

+        self.assertEqual(bar_dict["call_count"], 4)
+        self.assertGreaterEqual(bar_dict["total_time"], time1 * 4)
+        self.assertGreaterEqual(bar_dict["total_per_call"], time1)
+        self.assertGreaterEqual(bar_dict["cumulative_time"], time1 * 4)
+        self.assertGreaterEqual(bar_dict["cumulative_per_call"], time1)


The measurement error is too large to set a reasonable threshold, so I refer the above test function and use assertGreaterEqual.

ThreeMonth03 · 2024-07-06T19:27:36Z

tests/test_callprofiler.py

@@ -192,3 +192,95 @@ def foo():

        modmesh.call_profiler.reset()
        foo()
+


I rebase and squash another developing branch issue343 by git rebase -i [the history commit].

At the localhost, I run git checkout -b issue343_pr under the master branch to revert the latest solvcon /modmesh.

I merge the developing branch issue343 to this branch issue343_pr.

I forcely push the commit to origin/issue343_pr to modify the history.

I'm not sure whether I'm wrong to reduce and delete the unnecessary commit.

Your history contains commits from @tigercosmos @j8xixo12 @YenPeiChen07. That does not look right. Please rebase against master, remove commits not belonging to you, and squash small commits.

Your history contains commits from @tigercosmos @j8xixo12 @YenPeiChen07. That does not look right. Please rebase against master, remove commits not belonging to you, and squash small commits.

Fix.

It looks good now. Thanks.

yungyuc

The code is getting better. Some maintenance issues remain:

Commit history is not clean. Please rebase and squash to make it linear and avoid unnecessarily small commits. It is OK to leave a single commit after squashing, which will be trivially linear.
TimeRegistry::detailed_report() function body should not be included in class declaration.

yungyuc · 2024-07-07T12:22:29Z

cpp/modmesh/toggle/pymod/wrap_profile.cpp

+            .def(
+                "stat",
+                [](CallProfiler & profiler)
+                {
+                    std::stringstream ss;
+                    profiler.print_statistics(ss);
+                    return ss.str();
+                })
+            .def("result", &result)


Bravo! Thanks a lot.

yungyuc · 2024-07-07T12:30:17Z

tests/test_callprofiler.py

@@ -192,3 +192,95 @@ def foo():

        modmesh.call_profiler.reset()
        foo()
+


Your history contains commits from @tigercosmos @j8xixo12 @YenPeiChen07. That does not look right. Please rebase against master, remove commits not belonging to you, and squash small commits.

yungyuc · 2024-07-07T12:32:02Z

cpp/modmesh/toggle/profile.hpp

@@ -156,6 +166,39 @@ class TimeRegistry
        return ostm.str();
    }

+    std::string detailed_report() const


This function body is long. If we do not give it an implementation file (.cpp), it should be taken outside the class declaration.

(If you are not sure what does it mean, see #377 (comment) .)

ThreeMonth03

Commit history is not clean. Please rebase and squash to make it linear and avoid unnecessarily small commits. It is OK to leave a single commit after squashing, which will be trivially linear.
TimeRegistry::detailed_report() function body should not be included in class declaration.

@yungyuc I'm ready for next review.

ThreeMonth03 · 2024-07-07T23:00:18Z

tests/test_callprofiler.py

@@ -192,3 +192,95 @@ def foo():

        modmesh.call_profiler.reset()
        foo()
+


Your history contains commits from @tigercosmos @j8xixo12 @YenPeiChen07. That does not look right. Please rebase against master, remove commits not belonging to you, and squash small commits.

Fix.

ThreeMonth03 · 2024-07-07T23:05:53Z

cpp/modmesh/toggle/profile.cpp

+/*
+ * Copyright (c) 2023, Yung-Yu Chen <[email protected]>
+ *
+ * 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 HOLDER 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.
+ */
+
+#include <modmesh/toggle/profile.hpp>
+
+namespace modmesh
+{
+
+std::string TimeRegistry::detailed_report() const
+{
+    std::ostringstream ostm;
+    /// Header
+    ostm
+        << std::setw(40) << total_call_count()
+        << " function calls in " << total_time()
+        << " seconds" << std::endl;
+    ostm
+        << std::endl
+        << std::setw(40) << "Function Name"
+        << std::setw(25) << "Call Count"
+        << std::setw(25) << "Total Time (s)"
+        << std::setw(25) << "Per Call (s)"
+        << std::setw(25) << "Cumulative Time (s)"
+        << std::setw(25) << "Per Call (s)"
+        << std::endl;
+
+    /// Body
+    for (auto it = m_entry.begin(); it != m_entry.end(); ++it)
+    {
+        ostm
+            << std::setw(40) << it->first
+            << std::setw(25) << it->second.count()
+            << std::setw(25) << it->second.time()
+            << std::setw(25) << it->second.time() / it->second.count()
+            << std::setw(25) << it->second.ctime()
+            << std::setw(25) << it->second.ctime() / it->second.count()
+            << std::endl;
+    }
+    return ostm.str();
+}
+
+} /* end namespace modmesh */
+
+// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:


I had tried to define the function in profile.hpp, but it returned the error with multiple definitions of function. Therefore, I add a new file profile.cpp to avoid the error message.

When putting outside the declaration in the header/include file, a function needs to be made inline to avoid multiple definition when the header file is included in multiple compilation units: https://learn.microsoft.com/en-us/cpp/cpp/overview-of-member-functions?view=msvc-170 .

In this PR it is OK to use either inline or an implementation .cpp file for the definition of detailed_report(). When using the implementation file, it would be nice to follow the guideline of IWYU, but we can worry about it in the future.

When putting outside the declaration in the header/include file, a function needs to be made inline to avoid multiple definition when the header file is included in multiple compilation units: https://learn.microsoft.com/en-us/cpp/cpp/overview-of-member-functions?view=msvc-170 .

In this PR it is OK to use either inline or an implementation .cpp file for the definition of detailed_report(). When using the implementation file, it would be nice to follow the guideline of IWYU, but we can worry about it in the future.

Thanks, it is a very useful suggestion.

yungyuc

Thanks @ThreeMonth03 to make everything in a clear commit 3f18375 . It can use a complete and concise commit log. You can take a look at https://github.com/solvcon/modmesh/pull/360/commits for an example for writing the commit log. Your current log does not help a maintainer to understand what you did:

Recover the report and add detailed report in RegistryEntry

Fix the CI warning.

Fix the CI warning.

Expose the info of gtest.

Fix the reading bug of function.

Split the long line and remove redundant comment.

Fix the format, and test by python

Reduce the long line for python

Add the tolerance for time threshold

Add the tolerance for time threshold

Add the tolerance for time threshold

Fix the endl

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

modify review code

yungyuc · 2024-07-07T23:55:07Z

cpp/modmesh/toggle/profile.cpp

+/*
+ * Copyright (c) 2023, Yung-Yu Chen <[email protected]>
+ *
+ * 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 HOLDER 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.
+ */
+
+#include <modmesh/toggle/profile.hpp>
+
+namespace modmesh
+{
+
+std::string TimeRegistry::detailed_report() const
+{
+    std::ostringstream ostm;
+    /// Header
+    ostm
+        << std::setw(40) << total_call_count()
+        << " function calls in " << total_time()
+        << " seconds" << std::endl;
+    ostm
+        << std::endl
+        << std::setw(40) << "Function Name"
+        << std::setw(25) << "Call Count"
+        << std::setw(25) << "Total Time (s)"
+        << std::setw(25) << "Per Call (s)"
+        << std::setw(25) << "Cumulative Time (s)"
+        << std::setw(25) << "Per Call (s)"
+        << std::endl;
+
+    /// Body
+    for (auto it = m_entry.begin(); it != m_entry.end(); ++it)
+    {
+        ostm
+            << std::setw(40) << it->first
+            << std::setw(25) << it->second.count()
+            << std::setw(25) << it->second.time()
+            << std::setw(25) << it->second.time() / it->second.count()
+            << std::setw(25) << it->second.ctime()
+            << std::setw(25) << it->second.ctime() / it->second.count()
+            << std::endl;
+    }
+    return ostm.str();
+}
+
+} /* end namespace modmesh */
+
+// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:


When putting outside the declaration in the header/include file, a function needs to be made inline to avoid multiple definition when the header file is included in multiple compilation units: https://learn.microsoft.com/en-us/cpp/cpp/overview-of-member-functions?view=msvc-170 .

In this PR it is OK to use either inline or an implementation .cpp file for the definition of detailed_report(). When using the implementation file, it would be nice to follow the guideline of IWYU, but we can worry about it in the future.

yungyuc · 2024-07-08T00:07:02Z

tests/test_callprofiler.py

@@ -192,3 +192,95 @@ def foo():

        modmesh.call_profiler.reset()
        foo()
+


It looks good now. Thanks.

Add detailed report function in RegistryEntry Write the pytest for statistic function

ThreeMonth03 · 2024-07-08T10:03:01Z

Thanks @ThreeMonth03 to make everything in a clear commit 3f18375 . It can use a complete and concise commit log. You can take a look at https://github.com/solvcon/modmesh/pull/360/commits for an example for writing the commit log. Your current log does not help a maintainer to understand what you did:
Recover the report and add detailed report in RegistryEntry

Fix the CI warning.

Fix the CI warning.

Expose the info of gtest.

Fix the reading bug of function.

Split the long line and remove redundant comment.

Fix the format, and test by python

Reduce the long line for python

Add the tolerance for time threshold

Add the tolerance for time threshold

Add the tolerance for time threshold

Fix the endl

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

modify review code

Fix.
@yungyuc I've modified the comment messages.

ThreeMonth03 · 2024-07-08T10:46:44Z

Thanks @ThreeMonth03 to make everything in a clear commit 3f18375 . It can use a complete and concise commit log. You can take a look at https://github.com/solvcon/modmesh/pull/360/commits for an example for writing the commit log. Your current log does not help a maintainer to understand what you did:
Recover the report and add detailed report in RegistryEntry

Fix the CI warning.

Fix the CI warning.

Expose the info of gtest.

Fix the reading bug of function.

Split the long line and remove redundant comment.

Fix the format, and test by python

Reduce the long line for python

Add the tolerance for time threshold

Add the tolerance for time threshold

Add the tolerance for time threshold

Fix the endl

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

Fix the review commit

modify review code
Fix. @yungyuc I've modified the comment messages.

@yungyuc The github action is finished, and you could review the code when you are available.

yungyuc · 2024-07-08T11:01:49Z

LGTM. Thanks, @ThreeMonth03 .

ThreeMonth03 commented Jun 24, 2024

View reviewed changes

ThreeMonth03 mentioned this pull request Jun 25, 2024

Write the coding-style guide for Modmesh #362

Closed

yungyuc requested changes Jun 25, 2024

View reviewed changes

yungyuc assigned ThreeMonth03 Jun 25, 2024

yungyuc added enhancement New feature or request performance Profiling, runtime, and memory consumption labels Jun 25, 2024

ThreeMonth03 commented Jul 1, 2024

View reviewed changes

yungyuc requested changes Jul 3, 2024

View reviewed changes

ThreeMonth03 force-pushed the issue343_pr branch 2 times, most recently from 2e1c8a3 to 0e31b7b Compare July 6, 2024 18:39

ThreeMonth03 commented Jul 6, 2024

View reviewed changes

yungyuc requested changes Jul 7, 2024

View reviewed changes

ThreeMonth03 force-pushed the issue343_pr branch 2 times, most recently from 26a0708 to 3f18375 Compare July 7, 2024 22:59

ThreeMonth03 commented Jul 7, 2024

View reviewed changes

yungyuc requested changes Jul 8, 2024

View reviewed changes

Add the statistic function in callprofiler

4b0c17a

Add detailed report function in RegistryEntry Write the pytest for statistic function

ThreeMonth03 force-pushed the issue343_pr branch from 3f18375 to 4b0c17a Compare July 8, 2024 08:50

ThreeMonth03 closed this Jul 8, 2024

ThreeMonth03 reopened this Jul 8, 2024

yungyuc approved these changes Jul 8, 2024

View reviewed changes

yungyuc merged commit 14b3236 into solvcon:master Jul 8, 2024
26 checks passed

yungyuc linked an issue Jul 8, 2024 that may be closed by this pull request

Prototype profiler object serialization using JSON #343

Open

2 tasks

ThreeMonth03 deleted the issue343_pr branch July 14, 2024 14:44

ThreeMonth03 mentioned this pull request Sep 5, 2024

Enhance the function CallProfiler::print_statistics in the profiler. #421

Open

		@@ -192,3 +192,95 @@ def foo():

		modmesh.call_profiler.reset()
		foo()

Add the statistic function in the profiler #376

Add the statistic function in the profiler #376

Conversation

ThreeMonth03 commented Jun 24, 2024

ThreeMonth03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThreeMonth03 left a comment

Choose a reason for hiding this comment

ThreeMonth03 Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThreeMonth03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

ThreeMonth03 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThreeMonth03 Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThreeMonth03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThreeMonth03 commented Jul 8, 2024 • edited Loading

ThreeMonth03 commented Jul 8, 2024 • edited Loading

yungyuc commented Jul 8, 2024

ThreeMonth03 Jun 24, 2024 •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc Jul 3, 2024 •

edited

Loading

ThreeMonth03 left a comment •

edited

Loading

ThreeMonth03 Jul 6, 2024 •

edited

Loading

yungyuc left a comment •

edited

Loading

ThreeMonth03 commented Jul 8, 2024 •

edited

Loading

ThreeMonth03 commented Jul 8, 2024 •

edited

Loading