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

Jump to bottom

port: updated PhysicalCoreID() #2260

Closed
wants to merge 1 commit into from
Closed

port: updated PhysicalCoreID() #2260

wants to merge 1 commit into from

Conversation

joscollin
Copy link
Contributor

@joscollin joscollin commented May 6, 2017 •
edited

Summary:
Updated PhysicalCoreID() to use sched_getcpu() on x86_64 for glibc >= 2.22. Added a new
function named GetCPUID() that calls sched_getcpu(), to avoid repeated code. This change is done as per the comments of PR: #2230

Signed-off-by: Jos Collin jcollin@redhat.com

@facebook-github-bot
Copy link
Contributor

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

ajkr
ajkr requested changes May 9, 2017
View reviewed changes
Copy link
Contributor

@ajkr ajkr left a comment •
edited

Choose a reason for hiding this comment

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

can you change it to __GNUC_MINOR__ >= 22? I didn't get why not support 2.22.0 (and also __GNUC_PATCHLEVEL__ seems only defined in glibc 3+)

@ajkr
Copy link
Contributor

ajkr commented May 9, 2017 •
edited

By the way, I profiled this code (with glibc 2.23) and it makes GetPhysicalCore() 10x faster, as the comment originally suggested. This is really useful now as I'm working on core-local stats, which will make a lot of calls to GetPhysicalCore(). Thanks for your contribution!

@joscollin
Copy link
Contributor Author

@ajkr I'm happy to know that this PR makes performance improvements.

can you change it to GNUC_MINOR >= 22? I didn't get why not support 2.22.0

Sorry, that was a mistake from my side. I meant to include 2.22.0. Thanks for catching it.

(and also GNUC_PATCHLEVEL seems only defined in glibc 3+)

Are you suggesting me to avoid GNUC_PATCHLEVEL completely ?

Like this:

#if defined(__x86_64__) && (__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 22))

@ajkr
Copy link
Contributor

ajkr commented May 10, 2017

yup your proposed change looks good!

@facebook-github-bot
Copy link
Contributor

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

@joscollin
port: updated PhysicalCoreID()
c6fa468 
Summary:
Updated PhysicalCoreID() to use sched_getcpu() on x86_64 for glibc >= 2.22.  Added a new
function named GetCPUID() that calls sched_getcpu(), to avoid repeated code. Fixed the review
comments too.

Signed-off-by: Jos Collin <jcollin@redhat.com>
@facebook-github-bot
Copy link
Contributor

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

ajkr
ajkr approved these changes May 10, 2017
View reviewed changes
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM!

@joscollin
Copy link
Contributor Author

@ajkr Thanks for the approval. I hope we don't have to check for GNUC_PATCHLEVEL for glibc 3+ separately.

Also, I wonder why those functions are not inside a class ? Just like C-Style code ?

@facebook-github-bot
Copy link
Contributor

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

@ajkr
Copy link
Contributor

ajkr commented May 10, 2017 •
edited

shouldn't need to check patch level because everything in 3+ should have the proper support.

not sure why no class. I'd guess because these functions don't need to own/operate on any data so there's not much benefit to a class (besides namespacing).

@joscollin
Copy link
Contributor Author

@ajkr Could you please merge this ? I have a conflict now.

@ajkr
Copy link
Contributor

ajkr commented May 12, 2017

it was merged here: a620966

@joscollin
Copy link
Contributor Author

@ajkr Usually, the merges are done to the master branch. In this case, The code in the master branch is different. That's the reason I'm getting a merge conflict.
https://github.com/facebook/rocksdb/blob/master/port/port_posix.cc

Please suggest what should I do to get rid of the merge conflict.

Thanks.

@scv119
Copy link
Contributor

scv119 commented May 17, 2017

hmm seems this broke the mac build:

port/port_posix.cc:142:15: error: use of undeclared identifier 'sched_getcpu'
  int cpuno = sched_getcpu();

@sagar0
Copy link
Contributor

sagar0 commented May 18, 2017

@scv119 mac build is fixed in #2272 , as we discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajkr ajkr ajkr approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@joscollin @facebook-github-bot @ajkr @scv119 @sagar0