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

Reorder and align LRUCacheShard data members to reduce false sharing #2568

Closed

Conversation

IslamAbdelRahman
Copy link
Contributor

Reorder data members in LRUCacheShard so that the members that we update frequently don't share the same cache line with members that are mostly read-only

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@siying
Copy link
Contributor

siying commented Jul 11, 2017

Err... Windows doesn't support this...

@siying
Copy link
Contributor

siying commented Jul 11, 2017

@IslamAbdelRahman can you check whether it is really aligned in GCC too?

@grooverdan
Copy link
Contributor

grooverdan commented Jul 12, 2017

Also wouldn't you also align the highly accessed members at the end so they don't do false sharing with each other.

For attributes:

For Windows:  __declspec(align(n))
gcc/clang: _attribute__((__aligned__((n))))

@IslamAbdelRahman
Copy link
Contributor Author

Thanks @grooverdan for the attributes

You mean usage_, lru_usage_ and mutex each have it's own cache line ?

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman updated the pull request - view changes - changes since last import

@grooverdan
Copy link
Contributor

grooverdan commented Jul 13, 2017

Yes marking attributes to their own l1 cache line is what I had in mind as by the description they are heavily written. You can test with perf c2c

@grooverdan
Copy link
Contributor

Looks like you have having trouble with heap allocations. Seems like overloading new with aligned alloc is needed.

Here are some definitions that may help.

https://github.com/MariaDB/server/pull/185/files#diff-05fed5ee53d1f9b413d7468e7699ecb0R21

@siying
Copy link
Contributor

siying commented Jul 14, 2017

@IslamAbdelRahman still not supported on Windows.

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@IslamAbdelRahman
Copy link
Contributor Author

I just disabled it for windows, now it pass :D

Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

May consider @grooverdan 's suggestion too. But either way, it is good to go.

Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

Actually, what we want is to align since elems_, as it is the beginning of frequently changing parameter and from there they are exactly 64 bytes. This is hard to do with the alignment annotation.

@IslamAbdelRahman
Copy link
Contributor Author

abandoned for the sake of #2620

facebook-github-bot pushed a commit that referenced this pull request Jul 24, 2017
Summary:
combining #2568 and #2612.
Closes #2620

Differential Revision: D5464394

Pulled By: IslamAbdelRahman

fbshipit-source-id: 9f71d3058dd6adaf02ce3b2de3a81a1228009778
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.

4 participants