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

Add ankerl::unordered_dense containers #1881

Merged
merged 11 commits into from
Nov 27, 2023
Merged

Conversation

mwilsnd
Copy link
Collaborator

@mwilsnd mwilsnd commented Nov 21, 2023

Add ankerl::unordered_dense containers as a default build option, replacing std containers in various hot-paths. This yields a notable performance improvement on Android and Windows platforms.

@mwilsnd mwilsnd self-assigned this Nov 21, 2023
Copy link

github-actions bot commented Nov 21, 2023

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0% +6.62Ki  +0.1% +40.9Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-1881-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +19% +22.3Mi  +403% +24.1Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-1881-compared-to-legacy.txt

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Nov 22, 2023

@ntadej @louwers I think its time to retire GCC 8. Ran into what appears to be a defect in std::pmr with GCC 8 alone - trying to use dynamic_cast when RTTI is off. See this run.

No other compilers tested (MSVC 17 2022, Clang 11, 14, GCC 10, 12) show the same defect. I've temporarily forked this library to not try and include PMR if RTTI is off but before this is merged I want my temporary fork reverted.

@ntadej
Copy link
Collaborator

ntadej commented Nov 22, 2023

Which platform is that?

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Nov 22, 2023

Looks like Ubuntu.

@ntadej
Copy link
Collaborator

ntadej commented Nov 22, 2023

Can you just revert the fork then? I'll have a look and probably then we can either try to update the compiler or disable the test.

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Nov 22, 2023

I've reverted the fork modification that addressed GCC 8.

@ntadej
Copy link
Collaborator

ntadej commented Nov 22, 2023

Can you please try updating .github/actions/qt5-build/Dockerfile and set the image to FROM ghcr.io/maplibre/linux-builder:centos8-latest? I think this should fix it. I can later prepare an image with older CMake version so we have sufficient test coverage.

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Nov 22, 2023

Changed the image but no dice. I think as long as GCC 8 is trying to build this the workflow will fail.

@ntadej
Copy link
Collaborator

ntadej commented Nov 22, 2023

Hmm, I thought this image has a newer compiler. Let me update it tomorrow.

@ntadej
Copy link
Collaborator

ntadej commented Nov 26, 2023

You can rebase now.

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Nov 27, 2023

Awesome! Thanks for taking a look.

@mwilsnd mwilsnd marked this pull request as ready for review November 27, 2023 17:15
@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Nov 27, 2023

unordered_dense:
Americana: average=433.2634962933208, low1p=43.7258065742146
FB Lite: average=396.38147783531434, low1p=49.68743898979988

main:
Americana: 389.3204851308447, low1p=44.543213824095574
FB Lite: average=369.38082679297503, low1p=47.689207747774624

@mwilsnd mwilsnd merged commit 04d5695 into maplibre:main Nov 27, 2023
29 checks passed
louwers pushed a commit to louwers/maplibre-native that referenced this pull request Nov 30, 2023
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants