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

Prototype JSON serializer for CallProfiler #323

Merged
merged 17 commits into from
Jun 5, 2024
Merged

Prototype JSON serializer for CallProfiler #323

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2a8b6dd
Finish the serialization of RadixTree
Apr 28, 2024
56b91e5
Try to fix the clang format
Apr 28, 2024
03d939a
Fix the clang format and string problem
Apr 28, 2024
e36c42e
Modify the CallProfilerSerializer and add the test fixture for serial…
May 5, 2024
b754b43
Try to fix the gtest function
May 5, 2024
67df4d9
Try to fix the example in gtest
May 5, 2024
6d8d20b
Merge branch 'solvcon:master' into debug
ThreeMonth03 May 5, 2024
8d95caa
Optimize the format of the serializer output.
Jun 2, 2024
31f0b8c
Merge branch 'debug' of https://github.com/ThreeMonth03/modmesh into …
Jun 2, 2024
1ef1eef
Casting the node of the address to the unsigned long
Jun 2, 2024
caf93e3
Format the file in gtest.
Jun 2, 2024
ba561cb
Add the comment in gtest.
Jun 2, 2024
4c3efc7
Merge branch 'solvcon:master' into debug
ThreeMonth03 Jun 3, 2024
974ad73
Finish the gtest of serializer
Jun 4, 2024
3f939b8
Reformat the code
Jun 4, 2024
a7180ca
Modify i++ to ++i
Jun 4, 2024
5f1eb66
Merge pull request #3 from ThreeMonth03/test
ThreeMonth03 Jun 4, 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
54 changes: 54 additions & 0 deletions cpp/modmesh/toggle/RadixTree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,60 @@ void CallProfiler::print_profiling_result(const RadixTreeNode<CallerProfile> & n
}
}

void CallProfiler::serialize(std::ostream & outstream) const
Copy link
Member

Choose a reason for hiding this comment

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

It's better to keep the serialization code outside the object to be serialized. Perhaps try to make a CallProfilerSerializer. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks good to me.
It can make CallProfiler compact.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ThreeMonth03 I think it's better to make the json parser as a separate class, so later other classes can utilize the json parser as well. Let me know if you would like to implement the generic json parser. I can help implement the parser if you don't have time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ThreeMonth03 I think it's better to make the json parser as a separate class, so later other classes can utilize the json parser as well. Let me know if you would like to implement the generic json parser. I can help implement the parser if you don't have time.

I think I'm unable to implement the generic json parser.
@tigercosmos Could you give me a hand?

{
outstream << R"({)";
outstream << R"("radix_tree": )";
serialize_radix_tree(outstream);
outstream << R"(})";
}

void CallProfiler::serialize_radix_tree(std::ostream & outstream) const
{
outstream << R"({)";
Copy link
Collaborator

@tigercosmos tigercosmos Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. define a json schema
  2. implement the data structure in RadpidJson (or any other library)
  3. use the library to dump the json format

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

  1. define a json schema
  2. implement the data structure in RadpidJson (or any other library)
  3. use the library to dump the json format

Does it mean that I need to install RapidJson library in .github/workflows ?

Copy link
Collaborator

@tigercosmos tigercosmos Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you need to install RadpidJson in CMake, so you can use it in cpp files. (of course, also update the CI files)
You can do a study first before opting in RapidJson, perhaps there is a more suitable library for us.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See more discussion here: https://discord.com/channels/730297880140578906/1234815972809117778

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Does it mean that I should survey and implement a general json parser class?

outstream << R"("nodes": [)";
serialize_radix_tree_nodes(*(m_radix_tree.get_current_node()), true, outstream);
outstream << R"(],)";
outstream << R"("current_node": )" << m_radix_tree.get_current_node()->key() << R"(,)";
outstream << R"("unique_id": )" << m_radix_tree.get_unique_node();
outstream << R"(})";
}

// NOLINTNEXTLINE(misc-no-recursion)
Copy link
Member

Choose a reason for hiding this comment

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

Avoid recursion from the implementation

void CallProfiler::serialize_radix_tree_nodes(const RadixTreeNode<CallerProfile> & node, bool is_first_node, std::ostream & outstream) const
Copy link
Member

Choose a reason for hiding this comment

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

It's more readable to use some comment showing examples of the serialized results.

{
// 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": )";
node.data().serialize(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"(})";
for (const auto & child : node.children())
{
// NOLINTNEXTLINE(misc-no-recursion)
serialize_radix_tree_nodes(*child, false, outstream);
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In order to avoid the multiprogramming, I implement these three functions under the CallProfiler.


} /* end namespace modmesh */

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
43 changes: 43 additions & 0 deletions cpp/modmesh/toggle/RadixTree.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,45 @@ struct CallerProfile
call_count++;
}

// It returns the json format.
void serialize(std::ostream & outstream) const
{
outstream << R"({)";
outstream << R"("start_time": )" << start_time.time_since_epoch().count() << R"(,)";
outstream << R"("caller_name": ")" << caller_name << R"(",)";
outstream << R"("total_time": )" << total_time.count() << R"(,)";
outstream << R"("call_count": )" << call_count << R"(,)";
outstream << R"("is_running": )" << is_running;
outstream << R"(})";
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The serialization of the CallerProfile.


// It deserialize the json format.
Copy link
Member

Choose a reason for hiding this comment

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

This comment block is unnecessary.

void deserialize(const std::string & json)
{
std::stringstream ss(json);
std::string info;
// start_time
std::getline(ss, info, ':');
std::getline(ss, info, ',');
start_time = std::chrono::high_resolution_clock::time_point(std::chrono::nanoseconds(std::stoll(info)));
// caller_name
std::getline(ss, info, ':');
std::getline(ss, info, ',');
caller_name = info;
// total_time
std::getline(ss, info, ':');
std::getline(ss, info, ',');
total_time = std::chrono::nanoseconds(std::stoll(info));
// call_count
std::getline(ss, info, ':');
std::getline(ss, info, ',');
call_count = std::stoi(info);
// is_running
std::getline(ss, info, ':');
std::getline(ss, info, ',');
is_running = std::stoi(info);
}

std::chrono::high_resolution_clock::time_point start_time;
std::string caller_name;
std::chrono::nanoseconds total_time = std::chrono::nanoseconds(0); /// use nanoseconds to have higher precision
Expand Down Expand Up @@ -261,8 +300,12 @@ class CallProfiler
reset();
}

void serialize(std::ostream & outstream) const;

private:
void print_profiling_result(const RadixTreeNode<CallerProfile> & node, const int depth, std::ostream & outstream) const;
void serialize_radix_tree(std::ostream & outstream) const;
void serialize_radix_tree_nodes(const RadixTreeNode<CallerProfile> & node, bool is_first_node, std::ostream & outstream) const;

private:
RadixTree<CallerProfile> m_radix_tree; /// the data structure of the callers
Expand Down
27 changes: 26 additions & 1 deletion gtests/test_nopython_callprofiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,5 +212,30 @@ TEST_F(CallProfilerTest, cancel)
EXPECT_EQ(radix_tree().get_unique_node(), 0);
}

TEST_F(CallProfilerTest, test_serialization)
{
pProfiler->reset();

foo1();

// Example:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use a fixture (which can be just a string in this test file) to compare with the serialization results?

Copy link
Member

Choose a reason for hiding this comment

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

Does this example comment block have the same content as your fixture string? If yes, this comment block should be removed. And as mentioned above, the fixture strings should use good indentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is.
I will remove this comment block.

// {
// "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
// }
// }

std::stringstream ss;
pProfiler->serialize(ss);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the simple example of the serialization of the CallProfiler.

Copy link
Collaborator

@tigercosmos tigercosmos Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you only ran the function, but didn't verify the output.

here is what you can do:

  1. have a answer json file
  2. run the function and see if the output is equal to the answer.
  3. optional, verify the result matching the json schema (see more here)

} // namespace detail
} // namespace modmesh
} // namespace modmesh
Loading