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

Jump to bottom

add rational why ament_index pkgs don't have explicit performance tests #65

Merged
merged 3 commits into from
Aug 20, 2020
Merged

add rational why ament_index pkgs don't have explicit performance tests #65

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da538d3
add rational why ament_index pkgs don't have explicit performance tests
dirk-thomas Aug 6, 2020
567dc52
fix spelling
dirk-thomas Aug 19, 2020
d0dbd70
feedback
dirk-thomas Aug 19, 2020
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
8 changes: 7 additions & 1 deletion ament_index_cpp/QUALITY_DECLARATION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,13 @@ A description of how coverage statistics are summarized from this page, can be f

### Performance [4.iv]

`ament_index_cpp` does not conduct performance tests.
An environment variable defines the prefix paths of such resource indices and the API has a time complexity of `O(n)` where `n` is the number of prefix paths.
Copy link

Choose a reason for hiding this comment

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

@dirk-thomas meta: one could argue that we should have tests that backup that claim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you create a test to actually do that? What kind of thresholds would you use?

Copy link

@hidmic hidmic Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'd probably benchmark queries with increasing values of n and check if the result of a linear regression reasonably (within one or two standard deviations) explains the measurements. We would need a big index to see through OS noise though, I give you that.

And actually, now that I think about it, perhaps that's the only kind of performance test that would make sense here.

Copy link
Contributor Author

@dirk-thomas dirk-thomas Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Measuring enough different N trying to figure out if the complexity is linear as promised would be possible. (Even though this sounds extremely costly to me.)

The time complexity to query information is either scaling linearly with the number of resource types or with the number of resources per type (depending on which dimension is requested).
If the content of a specific resource is retrieved the time complexity is linear to the size of the content as is the memory usage in that case since the content is returned to the caller.
The performance of the implementation is defined by the performance of the underlying filesystem functions and the implemented logic doesn't add any significant overhead.
Copy link

Choose a reason for hiding this comment

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

@dirk-thomas meta: while I agree the logic overhead is likely negligible compared to that of IO, the performance of ament_index queries has more to do with the way the index is structured than with the code itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is why I tried to describe the complexity of the different kind of queries available.

Arguably if there would be a more efficient way the resource index could be structured to make queries more efficient (which still satisfies the requirements) that might be something better discussed on the design document (https://github.com/ament/ament_cmake/blob/master/ament_cmake_core/doc/resource_index.md).

Copy link

Choose a reason for hiding this comment

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

Arguably if there would be a more efficient way the resource index could be structured to make queries more efficient

Sure, my point being that to say The performance of the implementation is defined by the performance of the underlying filesystem functions is not entirely correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate why?

Copy link

Choose a reason for hiding this comment

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

Re-reading this thread, I get the impression that we're using the term performance for different things (algorithmic complexity vs. runtime cost). Since you do mention time-complexity, perhaps rephrasing as:

Suggested change
The performance of the implementation is defined by the performance of the underlying filesystem functions and the implemented logic doesn't add any significant overhead.
The runtime cost of the implementation is dominated by the runtime cost of the underlying filesystem API, and the implemented logic doesn't add any significant overhead.

would constrain interpretation. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me - applied in both locations in d0dbd70.


From a usage point of view it is also expected that the resource index is commonly only queried during startup and not at runtime of a production system.
Therefore `ament_index_cpp` does not conduct explicit performance tests.

### Linters and Static Analysis [4.v]

Expand Down
8 changes: 7 additions & 1 deletion ament_index_python/QUALITY_DECLARATION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,13 @@ Current coverage statistics can be viewed [here](https://ci.ros2.org/job/ci_linu

### Performance [4.iv]

`ament_index_python` does not conduct performance tests. It is not yet defined if this package requires performance testing and how addresses this topic.
An environment variable defines the prefix paths of such resource indices and the API has a time complexity of `O(n)` where `n` is the number of prefix paths.
The time complexity to query information is either scaling linearly with the number of resource types or with the number of resources per type (depending on which dimension is requested).
If the content of a specific resource is retrieved the time complexity is linear to the size of the content as is the memory usage in that case since the content is returned to the caller.
The performance of the implementation is defined by the performance of the underlying filesystem functions and the implemented logic doesn't add any significant overhead.

From a usage point of view it is also expected that the resource index is commonly only queried during startup and not at runtime of a production system.
Therefore `ament_index_python` does not conduct explicit performance tests.

### Linters and Static Analysis [4.v]

Expand Down