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

[Perf] Use compile-time string hashes when dispatching update properties #9841

Closed
wants to merge 4 commits into from

Conversation

asklar
Copy link
Member

@asklar asklar commented Apr 16, 2022

Description

We currently implement a lot of our update logic and mapping property names to values by using string comparisons.
We can shrink the size on disk and speed things up by instead computing a hash of the property names and comparing the resulting hashes.
What's more, the hashes for the values we compare against, can be computed at compile time.

Type of Change

  • New feature (non-breaking change which adds functionality)

What

In this PR I am starting with creating the mechanism to do compile-time string hashes, and applying it to one function (StyleYogaNode). We can then start opting in more and more functions (esp. UpdateProperties and its friends).

Results:

✅ Code size for the StyleYogaNode function went down by 16% when I converted the outer comparisons (the property name).

✅It went further down when I also converted the property value comparisons, by a total of 47%.

Baseline 6.8 kB
64-bit FNV hash 6.3 kB
32-bit FNV hash 5.7 kB
32-bit FNV hash with inner value compares 3.6 kB

Speed-wise, I ran some micro benchmarkings (with a reduced version of StyleYogaNode) and saw average time per call go from 52 ns to 42 ns, about 20% faster (caveat, the benchmark is a little flimsy so take this number as anecdotal for now...)

Microsoft Reviewers: Open in CodeFlow

}

template <size_t N>
struct fixed_string {
Copy link
Member

Choose a reason for hiding this comment

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

fixed_string

Could we Pascal casing for the names and files as we do in the rest of the code? It is better to keep common naming conventions.

return val[pos];
}
constexpr bool operator==(const char *v) const noexcept {
return strcmp(val, v) == 0;
Copy link
Member

Choose a reason for hiding this comment

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

strcmp

It cannot be constexpr, right?


} // namespace details

#define HASH_STRING(STR) details::fixed_string{STR}.hash()
Copy link
Member

Choose a reason for hiding this comment

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

HASH_STRING

It would be nice to avoid any such new global macros.
Instead, we can just define a simple constexpr function that does it.

Copy link
Member

Choose a reason for hiding this comment

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

Also, why to create the whole fixed_string here to calculate hash, why not just have a function that does it directly?

case HASH_STRING("alignItems"): {
YGAlign align;

switch (value_hash) {
Copy link
Member

Choose a reason for hiding this comment

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

switch

How can we be sure that we always get a unique hash for the given set of string values?

Why not just create a std::unordered_map to achieve the same results? It will take care about any possible hash duplication.

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.

2 participants