search

ament / ament_index

Apache License 2.0
13 stars 27 forks source link

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

Closed dirk-thomas closed 4 years ago

dirk-thomas commented 4 years ago

This is a first draft for a rational why performance tests aren't necessary for ament_index_* packages.

chapulina commented 4 years ago

I believe with these justifications, this package can be considered level 1 :smile: If that's indeed the case, do you mind updating the QD accordingly?

dirk-thomas commented 4 years ago

do you mind updating the QD accordingly?

I would rather get this PR approved and merged first and then do a follow up PR to bump the level (which needs a separate round of review if all criteria are satisfied).

hidmic commented 4 years ago

This is basically something the team has to decide: is it worth / necessary / useful to cover this API with performance tests.

Agreed. I cannot say if it's worth or not (though probably isn't compared to the rest of the system), but I honestly can't come up with any other way to justify dropping performance regression testing than to say we won't be changing the main algorithm any time soon.

hidmic commented 4 years ago

Another thought about whether to test this for performance or not. The problem is how REP-2004 frames performance testing. Perhaps we shouldn't be trying to justify testing or not for performance regressions in every package, but putting together a set of concrete use cases and benchmarking critical paths.

wjwwood commented 4 years ago

The problem is how REP-2004 frames performance testing.

What's the problem? It just says you need a policy. Your policy can be that you will not do performance testing or block releases based on their results. You just need to justify it.

Do you have a specific part of the REP in mind that is problematic?

hidmic commented 4 years ago

Quote:

However, if performance is a reasonable concern for use in a production system, there must be performance tests and they should be used in conjunction with a regression policy which aims to prevent unintended performance degradation.

On what basis can we justify that this package should not test for performance degradation? This patch describes ament_index queries' algorithmic complexity and runtime cost, plus a note on how these APIs are likely to be used, but I don't see how "no need for performance tests" logically follows.

FWIW I'm playing devil's advocate here on purpose, trying to make the same inquires a third-party that's interested in this Quality Declaration would make. Specially because IIUC this patch will be adapted to many other packages. Do not block on my concerns if you think that's not to worry about or that it is sufficient as it stands.

wjwwood commented 4 years ago

I think the core argument is that the answer to "if performance is a reasonable concern for use in a production system" is "no" for this package due to the way it is implemented and the places it is used.

hidmic commented 4 years ago

Ok, fair enough. Let's hinge on this is not likely to be part of a critical path in runtime.

dirk-thomas commented 4 years ago

(more importantly) ament_index_cpp is not used in performance critical parts of the ecosystem right now

I hope the current statement:

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.

covers the important reason well enough. Thanks for the feedback and discussion!