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.

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

Heuristic Benchmarking #278

Merged
merged 18 commits into from
Mar 26, 2023
Merged

Conversation

EliasLF
Copy link
Collaborator

@EliasLF EliasLF commented Mar 25, 2023

Description

Implements some benchmarks into the heuristic mapper (both for the whole mapping process, and for each layer):

  • number of generated nodes
  • number of expanded nodes
  • depth of the retrieved solution in the search tree (only for each layer, not globally for the whole mapping process)
  • average branching factor (i.e. how many children does a node generate on average)
  • effective branching factor (i.e. how many of those children on average were expanded before the solution was found; or in other words the branching factor, which a balanced, uniform tree would need to have, to contain the number of processed nodes (expanded + 1) with a depth equal to the solution depth)
  • the average time spent on each expanded node

All of this will help keeping track of performance changes, when implementing new heuristics or updating the existing heuristic.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@EliasLF EliasLF requested a review from burgholzer March 25, 2023 03:05
@codecov
Copy link

codecov bot commented Mar 25, 2023

Codecov Report

Merging #278 (a33373e) into main (732c8a8) will increase coverage by 0.0%.
The diff coverage is 92.7%.

@@          Coverage Diff          @@
##            main    #278   +/-   ##
=====================================
  Coverage   92.0%   92.0%           
=====================================
  Files         45      45           
  Lines       3813    3888   +75     
  Branches     638     650   +12     
=====================================
+ Hits        3508    3580   +72     
- Misses       305     308    +3     
Flag Coverage Δ
cpp 91.5% <92.6%> (+<0.1%) ⬆️
python 95.9% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/configuration/Configuration.hpp 100.0% <ø> (ø)
src/heuristic/HeuristicMapper.cpp 90.2% <87.5%> (-0.4%) ⬇️
include/MappingResults.hpp 78.4% <100.0%> (+3.9%) ⬆️
include/heuristic/HeuristicMapper.hpp 98.1% <100.0%> (+0.7%) ⬆️
mqt/qmap/compile.py 96.2% <100.0%> (+<0.1%) ⬆️
src/configuration/Configuration.cpp 87.8% <100.0%> (+0.3%) ⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Many thanks. I love the additional information this provides.
You can find a few small, nit-picky suggestions in the comments down below.

One bigger thing: have you tested how much the information tracking/computation affects the performance? Considering runtime, but also memory. Would it make sense to "hide" these computation behind a "debug" or "monitoring" or "benchmarking" flag in the configuration and only track the metrics if the flag is enabled? Or is the overhead negligible anyway and we might as well include it all the time?
I guess one of my main concern comes from the Results object being passed to Python which requires translating the C++ vector of results to Python. If the vector is large, this could be costly.

include/MappingResults.hpp Show resolved Hide resolved
include/heuristic/HeuristicMapper.hpp Outdated Show resolved Hide resolved
include/heuristic/HeuristicMapper.hpp Show resolved Hide resolved
src/heuristic/HeuristicMapper.cpp Outdated Show resolved Hide resolved
src/heuristic/HeuristicMapper.cpp Outdated Show resolved Hide resolved
test/test_heuristic.cpp Show resolved Hide resolved
test/test_heuristic.cpp Show resolved Hide resolved
test/test_heuristic.cpp Outdated Show resolved Hide resolved
test/test_heuristic.cpp Outdated Show resolved Hide resolved
test/test_heuristic.cpp Outdated Show resolved Hide resolved
@EliasLF
Copy link
Collaborator Author

EliasLF commented Mar 25, 2023

One bigger thing: have you tested how much the information tracking/computation affects the performance? Considering runtime, but also memory. Would it make sense to "hide" these computation behind a "debug" or "monitoring" or "benchmarking" flag in the configuration and only track the metrics if the flag is enabled? Or is the overhead negligible anyway and we might as well include it all the time? I guess one of my main concern comes from the Results object being passed to Python which requires translating the C++ vector of results to Python. If the vector is large, this could be costly.

Oh, I did not even think about Python here. Using C++ directly, I did not see any significant change in mapping time (however, I also did not check memory usage, though I do not think there is any potential for a huge memory overhead). But I guess it cannot hurt to hide the feature behind a debugging flag in the configuration. Thank you for the suggestion!

@burgholzer
Copy link
Member

One bigger thing: have you tested how much the information tracking/computation affects the performance? Considering runtime, but also memory. Would it make sense to "hide" these computation behind a "debug" or "monitoring" or "benchmarking" flag in the configuration and only track the metrics if the flag is enabled? Or is the overhead negligible anyway and we might as well include it all the time? I guess one of my main concern comes from the Results object being passed to Python which requires translating the C++ vector of results to Python. If the vector is large, this could be costly.

Oh, I did not even think about Python here. Using C++ directly, I did not see any significant change in mapping time (however, I also did not check memory usage, though I do not think there is any potential for a huge memory overhead). But I guess it cannot hurt to hide the feature behind a debugging flag in the configuration. Thank you for the suggestion!

I agree. It's probably not the worst thing (the data structure is rather tiny), but in order to maximize performance I believe it could be helpful to deactivate the metric tracking.

@burgholzer burgholzer added c++ Anything related to C++ code code quality Anything related to code quality and code style. labels Mar 26, 2023
Signed-off-by: Lukas Burgholzer <[email protected]>
calling `back()` on an empty vector is undefined behavior. This would happen whenever debug=false.
This commit also restructures the debug code so that it is more compact.

Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Just pushed some commits with finishing touches here.
Most importantly I took care of a case of undefined behavior (calling back() on an empty vector) and made the debug code in the mapper a little more compact/less spread out.
If CI checks turn all-green and you are ok with the changes I made, I'll proceed and merge.

@EliasLF
Copy link
Collaborator Author

EliasLF commented Mar 26, 2023

LGTM. Thank you for the refinements!

@burgholzer burgholzer merged commit f34a5dc into cda-tum:main Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code code quality Anything related to code quality and code style.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants