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

refactor: constexpr lookup tables #38771

Merged
merged 3 commits into from
Jun 14, 2023
Merged

refactor: constexpr lookup tables #38771

merged 3 commits into from
Jun 14, 2023

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jun 13, 2023 •
edited

Description of Change

Use constexpr lookup tables for some of our string-to-value lookups. Uses base::fixed_flat_map.

Checklist

Release Notes

Notes: none

@ckerr ckerr added the semver/patch backwards-compatible bug fixes label Jun 13, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 13, 2023
@ckerr ckerr changed the title Refactor/constexpr lookup tables refactor: constexpr lookup tables Jun 13, 2023
@dsanders11
Copy link
Member

Does constexpr actually add a benefit here? The values being looked up in the tables are still run-time values, I'm not sure the compiler can do any compile-time lookups. No harm in the usage of constexpr here AFAIK, just no tangible benefit.

@ckerr
Copy link
Member Author

ckerr commented Jun 13, 2023 •
edited

It doesn't have the same kind of compile-time-vs-heap-allocations benefits as the previous PR that replaced std::set<std::string>s, but it can significantly reduce object file size.

One good example is applying this approach to KeyboardCodeFromCharCode() shrinks the size of keyboard_util.o from 75016 bytes to 36896 bytes.

I wouldn't expect that level of benefit everywhere, e.g. in code that has fewer comparisons such as the network::mojom::RequestMode code.

@codebytere codebytere added target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jun 14, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 14, 2023
@jkleinsc jkleinsc merged commit bf1ba4a into main Jun 14, 2023
17 of 18 checks passed
@jkleinsc jkleinsc deleted the refactor/constexpr-lookup-tables branch June 14, 2023 21:00
@release-clerk
Copy link

release-clerk bot commented Jun 14, 2023

No Release Notes

@trop
Copy link
Contributor

trop bot commented Jun 14, 2023

I was unable to backport this PR to "25-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/25-x-y and removed target/25-x-y PR should also be added to the "25-x-y" branch. labels Jun 14, 2023
@trop trop bot mentioned this pull request Jun 14, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Jun 14, 2023

I have automatically backported this PR to "26-x-y", please check out #38797

@trop trop bot added in-flight/26-x-y merged/26-x-y PR was merged to the "26-x-y" branch. and removed target/26-x-y PR should also be added to the "26-x-y" branch. in-flight/26-x-y labels Jun 14, 2023
ckerr added a commit that referenced this pull request Jun 15, 2023
@ckerr
refactor: constexpr lookup tables (#38771)
8678291 
* refactor: use a constexpr lookup table in GetPathConstant()

* refactor: use a constexpr lookup table in SystemPreferences::GetColor()

* refactor: use a constexpr lookup table in SimpleURLLoaderWrapper::Create()
@ckerr ckerr mentioned this pull request Jun 15, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Jun 15, 2023

@ckerr has manually backported this PR to "25-x-y", please check out #38800

@trop trop bot added in-flight/25-x-y merged/25-x-y PR was merged to the "25-x-y" branch. and removed in-flight/25-x-y labels Jun 15, 2023
@trop trop bot mentioned this pull request Jun 15, 2023
Merged
trop bot added a commit that referenced this pull request Jun 15, 2023
@trop @ckerr
refactor: constexpr lookup tables (#38771)
c046869 
* refactor: use a constexpr lookup table in GetPathConstant()

* refactor: use a constexpr lookup table in SystemPreferences::GetColor()

* refactor: use a constexpr lookup table in SimpleURLLoaderWrapper::Create()

Co-authored-by: Charles Kerr <charles@charleskerr.com>
ckerr added a commit that referenced this pull request Jun 16, 2023
@ckerr
refactor: constexpr lookup tables (#38771)
cfb9519 
* refactor: use a constexpr lookup table in GetPathConstant()

* refactor: use a constexpr lookup table in SystemPreferences::GetColor()

* refactor: use a constexpr lookup table in SimpleURLLoaderWrapper::Create()
@ckerr ckerr mentioned this pull request Jun 16, 2023
Closed
3 tasks
@trop
Copy link
Contributor

trop bot commented Jun 16, 2023

@ckerr has manually backported this PR to "24-x-y", please check out #38815

@trop trop bot removed the in-flight/24-x-y label Jun 16, 2023
@ckerr ckerr mentioned this pull request Jun 16, 2023
Merged
3 tasks
@trop trop bot added the merged/24-x-y PR was merged to the "24-x-y" branch label Jun 16, 2023
codebytere pushed a commit that referenced this pull request Jun 16, 2023
@trop @ckerr
refactor: constexpr lookup tables (#38807)
053031e 
* refactor: constexpr lookup tables (#38771)

* refactor: use a constexpr lookup table in GetPathConstant()

* refactor: use a constexpr lookup table in SystemPreferences::GetColor()

* refactor: use a constexpr lookup table in SimpleURLLoaderWrapper::Create()

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix: use MakeFixedFlatMap() instead of MakeFixedFlatMapSorted()

The latter compiles faster but does not exist in 24-x-y

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/4296340

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
@commit-lint commit-lint bot mentioned this pull request Sep 6, 2023
Open
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@ckerr
refactor: constexpr lookup tables (electron#38771)
81db931 
* refactor: use a constexpr lookup table in GetPathConstant()

* refactor: use a constexpr lookup table in SystemPreferences::GetColor()

* refactor: use a constexpr lookup table in SimpleURLLoaderWrapper::Create()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@codebytere codebytere codebytere approved these changes

@dsanders11 dsanders11 dsanders11 approved these changes

Assignees
No one assigned
Labels
merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ckerr @dsanders11 @jkleinsc @codebytere