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.

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

Fix thread naming on Linux and Darwin #13569

Merged
merged 1 commit into from Mar 16, 2018
Merged

Fix thread naming on Linux and Darwin #13569

merged 1 commit into from Mar 16, 2018

Conversation

yol
Copy link
Member

@yol yol commented Feb 21, 2018

Description

Added some years ago, #define got lost in
autoconf -> cmake conversion.

Motivation and Context

It's very useful during debugging to be able to distinguish all the threads.

How Has This Been Tested?

Build/run on Linux

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@yol yol added Type: Fix non-breaking change which fixes an issue Platform: Linux v18 Leia labels Feb 21, 2018
@yol yol added this to the L 18.0-alpha1 milestone Feb 21, 2018
@yol yol self-assigned this Feb 21, 2018
@yol yol requested a review from jimfcarroll February 21, 2018 10:00
jimfcarroll
jimfcarroll approved these changes Feb 21, 2018
View reviewed changes
Copy link
Member

@jimfcarroll jimfcarroll left a comment

Choose a reason for hiding this comment

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

Nice. I like it.

@jimfcarroll
Copy link
Member

@FernetMenta, @notspiff I'm fine with this. It prefixes the thread names in GDB on pthreads supported systems. It's up to you guys.

@FernetMenta
Copy link
Contributor

max chars is 15. do we have thread names > 13 that would be truncated by this change?

@yol
Copy link
Member Author

yol commented Feb 21, 2018

max chars is 15. do we have thread names > 13 that would be truncated by this change?

Yes. But it only truncates a copy of the name, so it's not a change.

@FernetMenta
Copy link
Contributor

assume there is a thread name with 15 characters. Currently it would show up in i.e. top as
AudioengineSink
with this PR is would read:
k/AudioengineSi

Not really nice but not a big issue either. Just wanted to know if we have those cases.

@yol
Copy link
Member Author

yol commented Feb 21, 2018

Currently it would show up as "kodi" because the code has been broken for a few years already ;-)

And a limit is a limit. Thread names > 15 characters will be truncated anyway, and I do not think anyone is paying attention to that at the moment. I think scratching two characters off the limit is worth for seeing which threads are kodi-owned and which are not. On Linux we have a lot of threads owned by libraries such as mesa, pulseaudio etc. Also no one even noticed that it did not work at all for quite some time, so impact should be very limited.

@FernetMenta
Copy link
Contributor

I was usng this a lot as I worked on threads. You can configure top in a way that is shows all threads owned by an application tree-like under this app. I never felt the need for a prefix.
Anyway I don't mind losing two chars.

@yol
Copy link
Member Author

yol commented Feb 21, 2018

You can configure top in a way that is shows all threads owned by an application tree-like under this app.

Yes, that is exactly what this PR is for. Because it's been broken since the switch to CMake. The k/ prefix is just a sidenote.

@FernetMenta approve the PR if you're ok with it :-)

MrMC
MrMC reviewed Feb 21, 2018
View reviewed changes
xbmc/threads/platform/pthreads/ThreadImpl.cpp Outdated
std::string pthreadName("k/");
pthreadName += m_ThreadName;
// Limit on Linux is 15 chars + NULL
pthreadName.resize(15);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 27, 2018
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 2, 2018
@MartijnKaijser MartijnKaijser removed this from the Leia 18.0-alpha1 milestone Mar 3, 2018
@wsnipex
Copy link
Member

wsnipex commented Mar 3, 2018

is this still waiting for something?

@yol
Copy link
Member Author

yol commented Mar 3, 2018

@Rechi how's the OS X stuff? can you PR it?

@pkerling
Fix thread naming on Linux and Darwin
7cf42a4 
Added some years ago, #define got lost in
autoconf -> cmake conversion.
@yol yol changed the title Fix thread naming on Linux Fix thread naming on Linux and Darwin Mar 14, 2018
@yol yol merged commit b58a89c into xbmc:master Mar 16, 2018
@yol yol deleted the pthread_setname branch March 16, 2018 15:09
@Rechi Rechi added this to the Leia 18.0-alpha2 milestone Mar 16, 2018
@Rechi Rechi mentioned this pull request Apr 1, 2018
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FernetMenta FernetMenta FernetMenta approved these changes

@stefansaraev stefansaraev stefansaraev left review comments

@MrMC MrMC MrMC left review comments

@jimfcarroll jimfcarroll jimfcarroll approved these changes

@wsnipex wsnipex wsnipex approved these changes

@Rechi Rechi Rechi approved these changes

@notspiff notspiff Awaiting requested review from notspiff

Assignees

@yol yol

Labels
Platform: iOS Platform: Linux Platform: macOS Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Milestone
Leia 18.0-alpha2
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@yol @jimfcarroll @FernetMenta @wsnipex @stefansaraev @Rechi @MrMC @MartijnKaijser @jenkins4kodi @pkerling