Skip to content

[NSThread] Implement name property #807

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
Merged
merged 1 commit into from
Feb 7, 2017
Merged

[NSThread] Implement name property #807

merged 1 commit into from
Feb 7, 2017

Conversation

TheCodez
Copy link
Contributor

No description provided.

@parkera
Copy link
Contributor

parkera commented Jan 18, 2017

On Darwin, this calls a pthread non-posix API (pthread_setname_np) to give the name to the pthread library for debugging purposes. Is there an equivalent we can use on other platforms?

@TheCodez
Copy link
Contributor Author

That was my initial plan for the implementation, but seeing Darwins pthread_setname_np and pthread_getname_np not having a pthread_t parameter I wasn't quite sure how to implement it correctly.

Linux equivalents are:

int pthread_setname_np(pthread_t thread, const char *name);
int pthread_getname_np(pthread_t thread, char *name, size_t len);

@parkera
Copy link
Contributor

parkera commented Jan 18, 2017

On Darwin, the current thread is the implicit argument. We actually only set the name in pthread if the current thread is the same as the NSThread receiver.

@TheCodez TheCodez force-pushed the implement-nsthread-name branch from 9f44ffe to 6fc9e4a Compare January 18, 2017 23:27
@TheCodez
Copy link
Contributor Author

@parkera Implemented your suggestion.

@TheCodez TheCodez force-pushed the implement-nsthread-name branch 2 times, most recently from cde3288 to 1db8608 Compare January 18, 2017 23:46
@parkera
Copy link
Contributor

parkera commented Jan 19, 2017

Cool! Is it possible to get a test case along with this?

@TheCodez
Copy link
Contributor Author

TheCodez commented Jan 21, 2017 •
edited
Loading

@parkera somehow pthread_setname_np and pthread_getname_np aren't found on Ubuntu? Do you have any ideas?

[EDIT] Got it working by wrapping the function in CoreFoundation

@TheCodez TheCodez force-pushed the implement-nsthread-name branch 4 times, most recently from 592b1e9 to d688c49 Compare January 23, 2017 16:41
@parkera
Copy link
Contributor

parkera commented Jan 23, 2017

I believe those are darwin-only extensions. Is there anything equivalent in the pthread library on used on ubuntu?

@TheCodez
Copy link
Contributor Author

TheCodez commented Jan 23, 2017 •
edited
Loading

@parkera I got it working on Ubuntu. The problem was, that the functions weren't found in Swift. Calling them from CoreFoundation fixed the problem.

parkera
parkera reviewed Jan 23, 2017
View reviewed changes
CoreFoundation/Base.subproj/CFPlatform.c Outdated
@@ -1310,6 +1310,14 @@ _CFThreadRef _CFThreadCreate(const _CFThreadAttributes attrs, void *_Nullable (*
return thread;
}

void _CFThreadSetName(const char *_Nullable name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's decorate this with CF_SWIFT_EXPORT

parkera
parkera requested changes Jan 23, 2017
View reviewed changes
CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h Outdated
@@ -298,6 +298,8 @@ typedef pthread_t _CFThreadRef;

CF_EXPORT _CFThreadRef _CFThreadCreate(const _CFThreadAttributes attrs, void *_Nullable (* _Nonnull startfn)(void *_Nullable), void *restrict _Nullable context);

CF_EXPORT void _CFThreadSetName(const char *_Nullable name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

@TheCodez TheCodez force-pushed the implement-nsthread-name branch from d688c49 to 7a10e58 Compare January 23, 2017 19:40
@TheCodez
Copy link
Contributor Author

Done

@TheCodez
Copy link
Contributor Author

Should I add a test to the test case, to see if the pthread name is set correctly?

@parkera
Copy link
Contributor

parkera commented Jan 27, 2017

Sure, that'd be useful.

@TheCodez
Copy link
Contributor Author

Should I use const char* or CFStringRef in CF, e.g.

CF_SWIFT_EXPORT const char* _CFThreadGetName();
or:
CF_SWIFT_EXPORT CFStringRef _CFThreadGetName();

@parkera
Copy link
Contributor

parkera commented Jan 28, 2017

Generally we like to use the CF types for CF API. So CFStringRef

@TheCodez TheCodez force-pushed the implement-nsthread-name branch from 7a10e58 to f2c65de Compare January 28, 2017 22:52
@TheCodez
Copy link
Contributor Author

Sorry, I couldn't quite get CFStringRef to work right so I used the other approach, hope thats fine. Also I have one question: Why should those function be decorated with CF_SWIFT_EXPORT, when all other _CFThread functions are marked with CF_EXPORT?

@TheCodez TheCodez force-pushed the implement-nsthread-name branch 2 times, most recently from fb0f248 to 60aa182 Compare January 28, 2017 23:15
@TheCodez
Copy link
Contributor Author

@parkera could you test?

@parkera
Copy link
Contributor

parkera commented Feb 1, 2017

Sure, sorry about that.

@parkera
Copy link
Contributor

parkera commented Feb 1, 2017

@swift-ci please test

@TheCodez TheCodez force-pushed the implement-nsthread-name branch from 60aa182 to 3b83376 Compare February 7, 2017 16:24
@TheCodez
Copy link
Contributor Author

TheCodez commented Feb 7, 2017

@parkera I did some small improvements and cleanups to the test code. Could you review and test, please?

@parkera
Copy link
Contributor

parkera commented Feb 7, 2017

@swift-ci please test and merge

@swift-ci swift-ci merged commit b5e006c into swiftlang:master Feb 7, 2017
@TheCodez TheCodez deleted the implement-nsthread-name branch February 7, 2017 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@TheCodez @parkera @swift-ci