-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add Python wrap to CallProfiler
#315
Conversation
@@ -51,8 +51,8 @@ class RadixTreeNode | |||
static_assert(std::is_integral_v<key_type> && std::is_signed_v<key_type>, "signed integral required"); | |||
|
|||
RadixTreeNode(std::string const & name, key_type key) | |||
: m_name(name) | |||
, m_key(key) | |||
: m_key(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix Wreorder-ctor error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yungyuc THE PR is ready for review.
@@ -225,17 +225,16 @@ class CallProfiler | |||
~CallProfiler() = default; | |||
|
|||
// Called when a function starts | |||
void start_caller(const std::string & caller_name, std::function<void()> cancel_callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unused arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mark conversation as resolved. If you want to say it is resolved, leave a comment saying so.
Marking conversation as resolved makes it hard to keep track of discussion history.
@@ -265,14 +264,10 @@ class CallProfilerProbe | |||
{ | |||
public: | |||
CallProfilerProbe(CallProfiler & profiler, const char * caller_name) | |||
: m_profiler(profiler) | |||
, m_caller_name(caller_name) | |||
: m_caller_name(caller_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix Wreorder-ctor error
m_radix_tree.entry(caller_name); | ||
CallerProfile & callProfile = m_radix_tree.get_current_node()->data(); | ||
callProfile.caller_name = caller_name; | ||
callProfile.start_stopwatch(); | ||
} | ||
|
||
// Called when a function ends | ||
void end_caller(const std::string & caller_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unused arg
@@ -163,12 +163,43 @@ class MODMESH_PYTHON_WRAPPER_VISIBILITY WrapTimeRegistry | |||
|
|||
}; /* end class WrapTimeRegistry */ | |||
|
|||
class WrapCallProfiler : public WrapBase<WrapCallProfiler, CallProfiler> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expose CallProfiler to python so profiling util can be shared.
RadixTree is internal data structure, hence no need to expose this to python
tests/test_callprofiler.py
Outdated
foo1() | ||
result = modmesh.call_profiler.print_profiling_result() | ||
pattern = r"([-+]?\d*\.\d+|\d+) ms" | ||
match = re.search(pattern, result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is a good practice to extract value from the string and assert the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not. Please add relevant data accessors in CallProfiler
for the profiling data.
By writing tests, we will discover many things for improving the API (system) omitted when during the initial design. This "design-for-testing" is a powerful techniques for software development too, although it is not exactly the same as how it is used in IC design.
tests/test_callprofiler.py
Outdated
self.assertNotIn("foo1", result) | ||
|
||
@unittest.skipUnless("nt" != os.name, | ||
"timing code on windows has resoltion issue") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice there's time resolution issue in windows, causing the CI to fail as the measured time is always less than what it should be.
Wonder if there's a better way to handle this on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much much less it is in windows than in other systems?
Preferably this can be fixed (or worked around) in this PR. We use the skipUnless
trick if can't do it soon enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a workaround for Windows? e.g. increasing the time or enlarging the error tolerance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove functool comment in the test code.
- Add comment for the future plan of
profile_function
. - Use a little bit of time to resolve the windows timing issue for testing.
- Implement basic data access in
CallProfiler
for testing.
Again, please add in-line annotation to mark the points that you address or would like to discuss, and when everything is ready for review, leave a global comment. Combining the in-line comments and the global using the code review section/feature/whatever may be a good idea. Let's use this protocol from now on.
tests/test_callprofiler.py
Outdated
import modmesh | ||
|
||
|
||
# https://docs.python.org/3/library/functools.html#functools.wraps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the comment pointing to Python document. This is a simple helper in Python standard library that every Python developer should be familiar with, so I expect all contributors to modmesh to be familiar with it. We should not add comment for people who do not know how to learn easy things themselves.
On the other hand, if there are tricky interaction among popular API, or obscure bug, we usually add comments pointing to bug trackers.
|
||
|
||
# https://docs.python.org/3/library/functools.html#functools.wraps | ||
def profile_function(func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a helpful utility function for developing modmesh code, e.g., GUI (FYI @j8xixo12). Eventually I think a more tailored implementation should be made available somewhere under the modmesh
Python package.
At the time being, could you please add a comment for the above-mentioned argument?
When it comes to the to-be-useful-and-mature helper function in the future, I don't like the function name (profile_function
). But it is OK for a local helper in the test module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not seeing the descriptions in comment. Please point it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a helpful utility function for developing modmesh code, e.g., GUI (FYI @j8xixo12). Eventually I think a more tailored implementation should be made available somewhere under the
modmesh
Python package.At the time being, could you please add a comment for the above-mentioned argument?
By "add a comment for ...", I meant to add the comment in the code, so that a future maintainer will read it and perform the refactoring.
tests/test_callprofiler.py
Outdated
self.assertNotIn("foo1", result) | ||
|
||
@unittest.skipUnless("nt" != os.name, | ||
"timing code on windows has resoltion issue") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much much less it is in windows than in other systems?
Preferably this can be fixed (or worked around) in this PR. We use the skipUnless
trick if can't do it soon enough.
tests/test_callprofiler.py
Outdated
foo1() | ||
result = modmesh.call_profiler.print_profiling_result() | ||
pattern = r"([-+]?\d*\.\d+|\d+) ms" | ||
match = re.search(pattern, result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not. Please add relevant data accessors in CallProfiler
for the profiling data.
By writing tests, we will discover many things for improving the API (system) omitted when during the initial design. This "design-for-testing" is a powerful techniques for software development too, although it is not exactly the same as how it is used in IC design.
tests/test_callprofiler.py
Outdated
|
||
@unittest.skipUnless("nt" != os.name, | ||
"timing code on windows has resoltion issue") | ||
def test_single_fun_profile(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use func
as a shorthand for function
, but it's just my personal preference. I kind of feel fun
is for fun.
You don't have to change if you don't feel it the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func
makes more sense. If possible, I prefer spelling out the full name.
cpp/modmesh/toggle/RadixTree.hpp
Outdated
{ | ||
cancel(); | ||
}; | ||
m_profiler.start_caller(m_caller_name, cancel_callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you remove the cancel_ballback
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tigercosmos
Because the compiler raised unused variable error.
Also, I am not sure about the true behavior of this callback.
Could you please elaborate more or maybe kindly add some tests in gtests/test_nopython_callprofiler.cpp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@q40603 Originally, cancel_callback
is for canceling the profiling. For example, if we restart the profiler, we need to cancel the previous profiling, otherwise the profiler will have unexpected behavior. Surprisingly, I didn't fully support the cancel mechanism before. Let me file another PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check the PR:
#318
{ return wrapped_type::instance(); }) | ||
.def("start_caller", &wrapped_type::start_caller, py::arg("caller_name")) | ||
.def("end_caller", &wrapped_type::end_caller) | ||
.def("print_profiling_result", [](const CallProfiler & profiler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider taking a stream object as the argument.
@@ -31,6 +31,7 @@ | |||
#include <modmesh/base.hpp> | |||
#include <modmesh/buffer/buffer.hpp> | |||
#include <modmesh/toggle/profile.hpp> | |||
#include <modmesh/toggle/RadixTree.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to put the include here?
tests/test_callprofiler.py
Outdated
match = re.search(foo3_pattern, result) | ||
self.assertIsNotNone(match, result) | ||
foo3_total_time = float(match.group(1)) | ||
self.assertGreater(foo3_total_time, 3000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the number can be fixed. please refer to test_nopython_radixtree.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I am wrong here, should be test_nopython_callprofiler.cpp
40333e1
to
085d34a
Compare
I will try to finalize this PR in 2 days. @yungyuc @tigercosmos Hence, I tried to wrap the CallProfilerProbe instead, but I ran into a issue. Consider the following 2 code snippets: def CallProfileProbeA(func):
@wraps(func)
def wrapper(*args, **kwargs):
modmesh.CallProfilerProbe(func.__name__)
result = func(*args, **kwargs)
return result
return wrapper def CallProfileProbeB(func):
@wraps(func)
def wrapper(*args, **kwargs):
tmp = modmesh.CallProfilerProbe(func.__name__)
result = func(*args, **kwargs)
return result
return wrapper and a simple python code that drives the Profiler @CallProfileProbeA
def foo1():
time.sleep(2)
@CallProfileProbeA
def foo2():
time.sleep(3)
foo1()
if __name__ == '__main__':
foo2()
print(modmesh.call_profiler.print_profiling_result()) The two different implementations gave different result
For the CallProfileProbeA, the CallProfilerProbe object is deleted almost immediately because of the python garbage collection, reference counting of CallProfilerProbe is zero. (Spent a few hours debugging why CallProfileProbeA doesn't work 😂, also tried to implement CallProfileProbe as context manager, but give up.) I just wonder if there's a more elegant way for the CallProfilerProbe object to be persisted before the wrapper function return? |
@tigercosmos why did a helper in a profiler need to use callback? Callback is generally hard to understand and maintain. It does not sound like necessary. Is it possible to implement the similar capability without a callback? Or is there a strong reason to use a callback? |
Thanks, @q40603 @tigercosmos . Now there seem to be two things:
Can they be handled independently? If so, let's do it this way. If not, what is the dependency? |
If they are dependent, in my opinion, we can refactor the callback to another design, for now maybe we can skip supporting |
Thanks, @tigercosmos . Does that mean that @q40603 can continue the profiler wrapper work by skipping the wrapper of |
@yungyuc yes, I think the cancel function is not the priority. |
@yungyuc @tigercosmos |
@tigercosmos feel free to create a refactoring PR for discussing the proposal. It may conflict with what @q40603 is doing, but discussing with code is still better than only words. |
@q40603 Is there progress on this PR? |
CallProfiler
CallProfiler
CallProfiler
I am revising the data accessor of profiling result by making it returned in nested python dictionary format. |
It could be an issue to use Python dict for storing the data in the long run. It is slow. But we can do it in the short term like in this PR. Performance issues need to be addressed by profiling. |
d28d200
to
567dea3
Compare
remove unwanted checkin callprofiler driver code clang-format clang-format clang-format RadixTree flake8 floating number skip windows timeing test fix typo fix conflict wrap CallProfilerProbe wrap CallProfilerProbe profliing result in python dict profliing result in python dict add jsonschema assertGreaterEqual add MODMESH_PYTHON_WRAPPER_VISIBILITY busy loop fix flake8 use perf_counter flake8 jsonschema missing jsonschema
567dea3
to
1f07576
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove functool comment in the test code.
- Add comment for the future plan of
profile_function
.- Use a little bit of time to resolve the windows timing issue for testing.
- Implement basic data access in
CallProfiler
for testing.
@yungyuc, @tigercosmos The issues are addressed and the code is ready for review.
|
||
def busy_loop(duration): | ||
end_time = time.perf_counter() + duration | ||
while time.perf_counter() < end_time: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use time.perf_counter() to get higher time resolution instead of time.time()
result = modmesh.call_profiler.result() | ||
self.assertEqual(result, {}) | ||
|
||
def test_profiler_result_schema(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add schema validation using jsoncshema library.
self.assertEqual(second_bar_result["count"], 1) | ||
self.assertGreaterEqual(second_bar_result["total_time"], 500) | ||
|
||
def test_get_result_during_profiling(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the previous unit test all assert the result after profiling a series of function calls.
But user should be able to get the profiling result within the function too, so add this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
@@ -0,0 +1,41 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple json schema that regulates the output python dict format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if the schema should be put in this path.
we should also mention this schema in the cpp implementation.
.def("result", [](CallProfiler & profiler) | ||
{ | ||
const RadixTreeNode<CallerProfile> * root = profiler.radix_tree().get_root(); | ||
if (root->no_children()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid root only dict is returned when the radix tree is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may use some discussions, which can take place here or on the discord.
- Review and correct coding style
- Missing comments for the future plan of
profile_function()
.
cpp/modmesh/toggle/RadixTree.hpp
Outdated
@@ -72,6 +73,11 @@ class RadixTreeNode | |||
const T & data() const { return m_data; } | |||
const child_list_type & children() const { return m_children; } | |||
|
|||
bool no_children() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two options for this getter: no_children()
or has_children()
.
no_children()
acts like the implementation detail m_children.empty()
. If we choose this, what is the reason that we do not name the function as empty_children()
? empty
is more consistent to the style of STL.
Please consider the options and we should discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that no_chlidren should be renamed to empty_children, no_children is too verbal.
But I am curios, in practical, how to choose between negative check and positive check?
like empty_children or has_children in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time will tell.
# 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need two blank lines separating comment block and code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
# https://docs.python.org/3/library/functools.html#functools.wraps | ||
def profile_function(func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not seeing the descriptions in comment. Please point it out.
self.assertEqual(second_bar_result["count"], 1) | ||
self.assertGreaterEqual(second_bar_result["total_time"], 500) | ||
|
||
def test_get_result_during_profiling(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
tests/test_callprofiler.py
Outdated
def busy_loop(duration): | ||
end_time = time.perf_counter() + duration | ||
while time.perf_counter() < end_time: | ||
time.sleep(0.00001) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sleep
is usually not precise in a very short time. why not just use pass
for the busy loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think pass would be more straightforward.
Originally, the sleep is just for reducing the CPU usage.
return wrapper | ||
|
||
|
||
def busy_loop(duration): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's have a comment to explain the time unit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
fix coding style
d7af8db
to
0522d83
Compare
Issues addressed.
|
empty_children
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Still miss comments for the future plan of
profile_function()
.
|
||
|
||
# https://docs.python.org/3/library/functools.html#functools.wraps | ||
def profile_function(func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a helpful utility function for developing modmesh code, e.g., GUI (FYI @j8xixo12). Eventually I think a more tailored implementation should be made available somewhere under the
modmesh
Python package.At the time being, could you please add a comment for the above-mentioned argument?
By "add a comment for ...", I meant to add the comment in the code, so that a future maintainer will read it and perform the refactoring.
@q40603 Could you please find @terrychan999 for handling the failure of creating artifacts in the windows CI run? I think we should use some discussions to deal with it. Please bring it to the discord channel. |
Do you mean leaving comments in the code? |
Correct |
Comment added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yungyuc The code is ready for next review.
import modmesh | ||
|
||
|
||
# TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there (while I might find more to change later):
- Import the module and use . to access the functions in it.
import modmesh | ||
|
||
|
||
# TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
tests/test_callprofiler.py
Outdated
import unittest | ||
import time | ||
import json | ||
from jsonschema import validate, ValidationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please import the module and use .
to access the functions in it, like:
import jsonschema
...
jsonschema.validate(...)
Please review other Python files in the test and code directories and follow the style. @ThreeMonth03 this may be captured in the style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there (while I might find more to change later):
- Import the module and use . to access the functions in it.
Issues addressed, the code is ready for next review.
schema = json.load(schema_file) | ||
|
||
try: | ||
jsonschema.validate(instance=result, schema=schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix module import and use dot operator.
# 3. Try profile a real app that has both python and C++ codes | ||
# and add a unit test for this scenario. | ||
def profile_function(func): | ||
@functools.wraps(func) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix module import and use dot operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot. Merging.
Following the issue : #302
This PR includes :