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

Name spawned threads #145

Merged
merged 4 commits into from
Nov 20, 2023
Merged

Conversation

fornwall
Copy link
Collaborator

@fornwall fornwall commented Nov 18, 2023

Name spawned threads to make things more clear during debugging and profiling.

Max thread name is 15, so we can't fit more descriptive names like android-activity-main (but open for some other naming scheme).

Name spawned threads to make things more clear during debugging and
profiling.
@MarijnS95
Copy link
Member

This probably conflicts with #133, which IMO should be merged first to stay chronological.

@rib
Copy link
Collaborator

rib commented Nov 20, 2023

Nice.

Since we know we're running on Android maybe we can drop the 'android-' prefix even, unless the Java main thread is already called 'main' perhaps?

@fornwall
Copy link
Collaborator Author

Since we know we're running on Android maybe we can drop the 'android-' prefix even, unless the Java main thread is already called 'main' perhaps?

The reasoning for the android- prefix was to improve clarity for someone listing thread names for an application, just having main and stdio is a bit more nondescriptive/generic.

But how about:

  • native-activity and game-activity for the main threads - in that way one call also clearly tell if the game or native activity is used.
  • stdio-to-logcat as the stdio-forwarder thread - shows what it is doing.
    ?

@rib
Copy link
Collaborator

rib commented Nov 20, 2023

Yeah, stdio-to-logcat sounds a bit more descriptive of that thread's function.

I think main would be a pretty typical name for an application's main thread (and what I'd also find intuitive if I was looking at a list of threads in a profiler) but my main :) concern with that on Android is that that it's common to refer to the application's Java thread as the 'main' thread and maybe that's even what it's named at the system level?

I wouldn't be super keen on having 'activity' in the name since the Activity is really something that exists on the JVM side of things which might then be misleading.

Something like android_main might be appropriate if we need to disambiguate from a Java 'main' thread where this is the name of the actual fn android_main() native function that's called.

@fornwall
Copy link
Collaborator Author

fornwall commented Nov 20, 2023

I wouldn't be super keen on having 'activity' in the name since the Activity is really something that exists on the JVM side of things which might then be misleading.

Agreed!

Something like android_main might be appropriate if we need to disambiguate from a Java 'main' thread where this is the name of the actual fn android_main() native function that's called.

Sounds good - let's use android_main?

Pushed a commit to use android_main and stdio-to-logcat as thread names now.

@rib
Copy link
Collaborator

rib commented Nov 20, 2023

Okey, @MarijnS95's updated PR for #133 ended up naming the stdio-to-logcat thread as proposed here (it made sense to do the rename while anyway factoring out a shared utility for spawning that thread), so its just the android_main thread left to name here. Yeah I think 'android_main' seems reasonable to me based on the name of the top-level native function for that thread.

@fornwall
Copy link
Collaborator Author

Okey, @MarijnS95's updated PR for #133 ended up naming the stdio-to-logcat thread as proposed here (it made sense to do the rename while anyway factoring out a shared utility for spawning that thread), so its just the android_main thread left to name here

👍 - merge conflict has now been resolved.

Copy link
Collaborator

@rib rib left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me

@rib rib merged commit 2deec16 into rust-mobile:main Nov 20, 2023
3 checks passed
@fornwall fornwall deleted the android-main-thread-name branch November 22, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants