Prototype JSON serializer for CallProfiler
#323
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,132 @@ void CallProfiler::print_profiling_result(const RadixTreeNode<CallerProfile> & n | |
| } | ||
| } | ||
|
|
||
| void CallProfilerSerializer::serialize_call_profiler(const CallProfiler & profiler, std::ostream & outstream) | ||
| { | ||
| // Example: | ||
| // { | ||
| // "radix_tree": | ||
| // { | ||
| // "nodes":[ | ||
| // {"key":-1,"name":"","data":{"start_time": 0,"caller_name": "","total_time": 0,"call_count": 0,"is_running": 0},"children":[0]}, | ||
| // {"key":0,"name":"void modmesh::detail::foo1()","data":{"start_time": 17745276708555250,"caller_name": "void modmesh::detail::foo1()","total_time": 61002916,"call_count": 1,"is_running": 1},"children":[1]}, | ||
| // {"key":1,"name":"void modmesh::detail::foo2()","data":{"start_time": 17745276708555458,"caller_name": "void modmesh::detail::foo2()","total_time": 54002250,"call_count": 1,"is_running": 1},"children":[2]}, | ||
| // {"key":2,"name":"void modmesh::detail::foo3()","data":{"start_time": 17745276743555833,"caller_name": "void modmesh::detail::foo3()","total_time": 19001833,"call_count": 1,"is_running": 1},"children":[]} | ||
| // ], | ||
| // "current_node":-1, | ||
| // "unique_id":3 | ||
| // } | ||
| // } | ||
|
There was a problem hiding this comment. The json format is not informative enough. There was a problem hiding this comment. Additionally, the dictionary, that map the function to keyID, should be added into the radix tree. There was a problem hiding this comment. By the address of the node in the nodes, do you mean the address of the stack frame? Maybe it is worth a try, but I am not 100% sure the stack frame address can be used to identify a node. There was a problem hiding this comment.
Yes, it is. By the way, I try to casting the pointer to the unsigned long when serializing. There was a problem hiding this comment.
You are right. Here is a bug. |
||
|
|
||
| outstream << R"({)"; | ||
| outstream << R"("radix_tree": )"; | ||
| CallProfilerSerializer::serialize_radix_tree(profiler, outstream); | ||
| outstream << R"(})"; | ||
| } | ||
|
|
||
| void CallProfilerSerializer::serialize_radix_tree(const CallProfiler & profiler, std::ostream & outstream) | ||
| { | ||
| // Example: | ||
|
There was a problem hiding this comment. This also looks like unnecessary and unclear comments. |
||
| // { | ||
| // "nodes":[ | ||
| // {"key":-1,"name":"","data":{"start_time": 0,"caller_name": "","total_time": 0,"call_count": 0,"is_running": 0},"children":[0]}, | ||
| // {"key":0,"name":"void modmesh::detail::foo1()","data":{"start_time": 17745276708555250,"caller_name": "void modmesh::detail::foo1()","total_time": 61002916,"call_count": 1,"is_running": 1},"children":[1]}, | ||
| // {"key":1,"name":"void modmesh::detail::foo2()","data":{"start_time": 17745276708555458,"caller_name": "void modmesh::detail::foo2()","total_time": 54002250,"call_count": 1,"is_running": 1},"children":[2]}, | ||
| // {"key":2,"name":"void modmesh::detail::foo3()","data":{"start_time": 17745276743555833,"caller_name": "void modmesh::detail::foo3()","total_time": 19001833,"call_count": 1,"is_running": 1},"children":[]} | ||
| // ], | ||
| // "current_node":-1, | ||
| // "unique_id":3 | ||
| // } | ||
|
|
||
| outstream << R"({)"; | ||
|
There was a problem hiding this comment. I think you should not do the JSON serialization yourself. Consider using a library, such as RapidJson, to help you wrap the object into JSON format. Here is what you can do:
There was a problem hiding this comment.
Does it mean that I need to install RapidJson library in .github/workflows ? There was a problem hiding this comment. you need to install RadpidJson in CMake, so you can use it in cpp files. (of course, also update the CI files) There was a problem hiding this comment. I like @ThreeMonth03 's current approach. Developing the JSON writing code provides more freedom than using a thirdparty. A thirdparty introduces additional dependency and increases the complexity of the building system. JSON is easy, and writing a parser should be fun. I do not see a good reason to use a thirdparty JSON library. There was a problem hiding this comment. See more discussion here: https://discord.com/channels/730297880140578906/1234815972809117778 There was a problem hiding this comment. Per the discussion in Discord, I don't think we need a library for JSON, but if we need one, the current implementation is not general, we should have a JSON parser class instead. There was a problem hiding this comment.
Does it mean that I should survey and implement a general json parser class? |
||
| outstream << R"("nodes": [)"; | ||
| CallProfilerSerializer::serialize_radix_tree_nodes(profiler.radix_tree().get_current_node(), outstream); | ||
| outstream << R"(],)"; | ||
| outstream << R"("current_node": )" << profiler.radix_tree().get_current_node()->key() << R"(,)"; | ||
| outstream << R"("unique_id": )" << profiler.radix_tree().get_unique_node(); | ||
| outstream << R"(})"; | ||
| } | ||
| void CallProfilerSerializer::serialize_radix_tree_nodes(const RadixTreeNode<CallerProfile> * node, std::ostream & outstream) | ||
|
There was a problem hiding this comment. Keep a blank line between functions. Is it possible to make clang-format capture this? (Apparently we haven't configured it this way, yet.) There was a problem hiding this comment. No, I use the clang-format tools, and it doesn't work. |
||
| { | ||
| std::queue<const RadixTreeNode<CallerProfile> *> nodes_buffer; | ||
| nodes_buffer.push(node); | ||
| bool is_first_node = true; | ||
|
|
||
| // Serialize the nodes in a breadth-first manner. | ||
|
There was a problem hiding this comment. Good comment. It's informative and helps read the code. |
||
| while (!nodes_buffer.empty()) | ||
| { | ||
| const int nodes_buffer_size = nodes_buffer.size(); | ||
| for (int i = 0; i < nodes_buffer_size; i++) | ||
|
There was a problem hiding this comment. Use If |
||
| { | ||
| const RadixTreeNode<CallerProfile> * current_node = nodes_buffer.front(); | ||
| nodes_buffer.pop(); | ||
| CallProfilerSerializer::serialize_radix_tree_node(*current_node, is_first_node, outstream); | ||
| is_first_node = false; | ||
| for (const auto & child : current_node->children()) | ||
| { | ||
| nodes_buffer.push(child.get()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void CallProfilerSerializer::serialize_radix_tree_node(const RadixTreeNode<CallerProfile> & node, bool is_first_node, std::ostream & outstream) | ||
| { | ||
| // Example: | ||
|
There was a problem hiding this comment. This small example is informative, but the purpose is not clearly written in the comment. "Example" alone does not help. Please come up with clearer description for what you want to say to a maintainer (assuming they are not you). |
||
| // { | ||
| // "key":-1, | ||
| // "name":"", | ||
| // "data":{"start_time": 0,"caller_name": "","total_time": 0,"call_count": 0,"is_running": 0}, | ||
| // "children":[0] | ||
| // } | ||
|
|
||
| // Avoid the trailing comma. | ||
| if (!is_first_node) | ||
| { | ||
| outstream << R"(,)"; | ||
| } | ||
| outstream << R"({)"; | ||
| outstream << R"("key": )" << node.key() << R"(,)"; | ||
| outstream << R"("name": ")" << node.name() << R"(",)"; | ||
| outstream << R"("data": )"; | ||
| CallProfilerSerializer::serialize_caller_profile(node.data(), outstream); | ||
| outstream << R"(,)"; | ||
| outstream << R"("children": [)"; | ||
| bool is_first_child = true; | ||
| for (const auto & child : node.children()) | ||
| { | ||
| // Avoid the trailing comma. | ||
| if (!is_first_child) | ||
| { | ||
| outstream << R"(,)"; | ||
| } | ||
| is_first_child = false; | ||
| outstream << child->key(); | ||
| } | ||
| outstream << R"(])"; | ||
| outstream << R"(})"; | ||
| } | ||
|
|
||
| void CallProfilerSerializer::serialize_caller_profile(const CallerProfile & profile, std::ostream & outstream) | ||
| { | ||
| // Example: | ||
|
|
||
| // { | ||
| // "start_time": 0, | ||
| // "caller_name": "", | ||
| // "total_time": 0, | ||
| // "call_count": 0, | ||
| // "is_running": 0 | ||
| // } | ||
|
|
||
| outstream << R"({)"; | ||
| outstream << R"("start_time": )" << profile.start_time.time_since_epoch().count() << R"(,)"; | ||
| outstream << R"("caller_name": ")" << profile.caller_name << R"(",)"; | ||
| outstream << R"("total_time": )" << profile.total_time.count() << R"(,)"; | ||
| outstream << R"("call_count": )" << profile.call_count << R"(,)"; | ||
| outstream << R"("is_running": )" << profile.is_running; | ||
| outstream << R"(})"; | ||
| } | ||
|
|
||
| } /* end namespace modmesh */ | ||
|
There was a problem hiding this comment. I add the comment about the serialization format. There was a problem hiding this comment. It's good to use a loop to traverse. Is there a performance concern? There was a problem hiding this comment. I have no idea. It seems to be fine. |
||
|
|
||
| // vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4: | ||
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 do not need to keep examples everywhere. The example in this block of comments seems to duplicate with that in google test. Please keep the code here clean by removing the unnecessary comments.
And marking only "Example" in this block does not help a maintainer to read. The only common information between the code and comment in the function is
"radix_tree". It does not help maintainability.If anything in the example comments here should be kept, please make it concise. And try to use doxygen format if it fits.