-
Notifications
You must be signed in to change notification settings - Fork 65
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 license field in Stackage class. #66
Conversation
Thank you for working on this. Since the performance of |
Ok, I made simple scripts (that I also pushed in 6f3fa0b for sharing purpose) and did small benchmark tests. Following is the result of running With the change:
Without:
|
a885f10
to
e52d453
Compare
Testing more, actually I take back what I said in the previous post #66 (comment) -- there's no obvious latency using this PR. In some tests that use this PR actually are faster than without this PR. This time I ran the following command 500 times (all units in second):
Looking at the result I would say the time diff is negligible. I just don't understand why this PR doesn't slow it down. Some things I checked to verify that the test method is reasonable:
You can download and run the benchmark tests:
Entire strace files. [1]
[2]
[3]
[4]
[5]
[6]
|
include/rospack/rospack.h
Outdated
@@ -146,6 +146,7 @@ class ROSPACK_DECL Rosstackage | |||
bool crawled_; | |||
std::string name_; | |||
std::string tag_; | |||
std::vector<std::string> licenses_; |
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.
Since the new member is private it is not accessible by any external code. What is the purpose here?
Also this change will break ABI which needs to be considered to decide into which branch to merge 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.
Removed this line. I can't remember, but I think this was added simply by mistake.
test/benchmark/measure_performace.sh
Outdated
@@ -0,0 +1,47 @@ | |||
#!/bin/bash |
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 script itself can't be run. Therefore it should neither be executable nor have a shebang line. It should probably output a message how the functions can be used when it is being sourced.
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.
shebang removed.
The benchmark
folder and its content is not specific to rospack
at all so I can remove the commit that added it if that's better.
e52d453
to
755f007
Compare
755f007
to
bc6d495
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.
LGTM now.
Rosstackage
class does probably not necessarily map exactly all the elements defined inpackage.xml
, but from the api perspective, it'll be beneficial to be able to access more elements through this class so that client codes won't have to parse xml when needed (here this is related to #65).Possible downside is that whenever
rospack
crawls it could take longer since more parsing happens (though I haven't tested), and for particularly usecases where license info is not needed this added info is simply a waste of time. I'm not sure how critical rospack's performance is, so I appreciate opinions.