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

drt: Use boost flat hashmap when beneficial #6253

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sgizler
Copy link

@sgizler sgizler commented Nov 27, 2024

  • Bump boost to get flat hashtables
  • Replace std::set indexed by pair of Points with boost boost::unordered_flat_set
  • Replace std::map indexed by Point3D with boost::unordered_flat_map
  • Replace std::unordered_map with boost::unordered_flat_map (this was the single case, in which DRT already used hashtable rather than search tree)

Benchmarks of drt:

ibex, PDK: asap7

Branch Min [s] Max [s] Mean [s] Relative mean Median [s] Relative median
boost-flat-hashmap-opt 286.27 287.44 286.77 ± 0.35 1.00 ± 0.00 286.73 1.00
baseline 290.79 292.14 291.41 ± 0.41 1.02 ± 0.00 291.35 1.02
boost-1.86 290.49 292.40 291.36 ± 0.54 1.02 ± 0.00 291.26 1.02

ibex, PDK: nangate45

Branch Min [s] Max [s] Mean [s] Relative mean Median [s] Relative median
boost-flat-hashmap-opt 87.66 88.38 88.00 ± 0.17 1.00 ± 0.00 88.00 1.00
baseline 89.15 89.83 89.39 ± 0.22 1.02 ± 0.00 89.31 1.01
boost-1.86 89.11 89.51 89.33 ± 0.12 1.02 ± 0.00 89.31 1.01

black_parrot, PDK: nangate45

Branch Min [s] Max [s] Mean [s] Relative mean Median [s] Relative median
boost-flat-hashmap-opt 1035.30 1039.40 1036.90 ± 1.01 1.00 ± 0.00 1036.85 1.00
baseline 1056.01 1057.20 1056.48 ± 0.33 1.02 ± 0.00 1056.44 1.02
boost-1.86 1057.09 1058.94 1057.84 ± 0.54 1.02 ± 0.00 1057.72 1.02

@maliberty
Copy link
Member

Please fix the missing DCO (see the details like).

@vvbandeira
Copy link
Member

Please also merge or rebase the master branch into your branch to get the latest changes to CI so pr-head can build.

@sgizler sgizler force-pushed the boost-flat-hashmap-opt branch from 00e7666 to b9f8bef Compare November 28, 2024 12:48
@sgizler sgizler force-pushed the boost-flat-hashmap-opt branch from 825cef5 to 6798309 Compare November 28, 2024 12:54
@sgizler sgizler marked this pull request as draft November 28, 2024 18:53
Signed-off-by: Szymon Gizler <[email protected]>
@sgizler sgizler force-pushed the boost-flat-hashmap-opt branch from e536182 to e33d5b2 Compare November 29, 2024 12:44
It seems, that after rebasing on @93819d5, `node_map_` map started
being order-sensitive what prevents us from converting it to hashmap

Signed-off-by: Szymon Gizler <[email protected]>
@sgizler sgizler force-pushed the boost-flat-hashmap-opt branch from e33d5b2 to 6cd3fbe Compare November 29, 2024 12:47
@sgizler
Copy link
Author

sgizler commented Dec 10, 2024

Rebasing introduced regression (one map I converted before, is now used in way that seems to depend on element ordering of std::map). After regression fix, the gain unfortunately got slightly reduced:

ibex, PDK: asap7

Branch Min [s] Max [s] Mean [s] Relative mean Median [s] Relative median
boost-flat-hashmap-opt 325.45 327.16 326.17 ± 0.50 1.00 ± 0.00 326.11 1.00
master 327.00 330.13 328.58 ± 0.90 1.01 ± 0.00 328.55 1.01

ibex, PDK: nangate45

Branch Min [s] Max [s] Mean [s] Relative mean Median [s] Relative median
boost-flat-hashmap-opt 131.23 132.19 131.61 ± 0.34 1.00 ± 0.00 131.59 1.00
master 132.45 133.45 132.94 ± 0.31 1.01 ± 0.00 132.90 1.01

black_parrot, PDK: nangate45

Branch Min [s] Max [s] Mean [s] Relative mean Median [s] Relative median
boost-flat-hashmap-opt 1316.26 1319.54 1317.98 ± 1.04 1.00 ± 0.00 1318.08 1.00
master 1327.30 1330.67 1329.28 ± 0.99 1.01 ± 0.00 1329.08 1.01

I think it is ready for review now.

@sgizler sgizler marked this pull request as ready for review December 10, 2024 13:00
@vvbandeira vvbandeira requested a review from maliberty December 16, 2024 17:41
find_package(Boost REQUIRED)
find_package(Boost 1.81.0)
Copy link
Member

Choose a reason for hiding this comment

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

any reason to drop REQUIRED?

Copy link
Author

Choose a reason for hiding this comment

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

I did it order to provide custom error message with suggestion about rerunning dependency installer.

@maliberty
Copy link
Member

Are the results identical or does this require a full test

@sgizler
Copy link
Author

sgizler commented Dec 20, 2024

Results were identical in cases I checked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants