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

Fix Alignment of Hashed Structs #287

Merged
merged 4 commits into from
May 18, 2017
Merged

Fix Alignment of Hashed Structs #287

merged 4 commits into from
May 18, 2017

Conversation

Adlai-Holler
Copy link
Member

@Adlai-Holler Adlai-Holler commented May 18, 2017

  • We initialize our structs using { 7, 5, 3 }. When you do this, if the struct has padding, the memory contained in the padding bytes contains garbage.
  • Our ASImageNodeContentsKey hashes a struct that has padding bytes.
  • So that hash wasn't reliable, and our cache hit rate plummeted.

The solution is:

  • Enable the -Wpadded warning around structs that we use {} and subsequently hash.
  • Update the ASImageNodeContentsKey struct to be aligned (change BOOL to NSInteger).
  • Update the documentation on ASHashBytes to point out this warning.

We could enable -Wpadded globally but usually we don't care too much about padding so let's not do it for now.

This patch is extremely important and should be released in a hotfix. The image cache hit rate before this patch is extremely low, at least on my mac.

@Adlai-Holler Adlai-Holler merged commit d30c357 into master May 18, 2017
@Adlai-Holler Adlai-Holler deleted the AHFixHashing branch May 18, 2017 01:08
Adlai-Holler added a commit that referenced this pull request May 18, 2017
* Fix alignment of ASImageNodeContentsKey struct to fix hashing

* Change the change log by logging a change

* Add the world's stupidest explicit cast

* Actually its simpler
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
* Fix alignment of ASImageNodeContentsKey struct to fix hashing

* Change the change log by logging a change

* Add the world's stupidest explicit cast

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

Successfully merging this pull request may close these issues.

None yet

2 participants