Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Android specific thread pool that keeps threads attached to JVM #14450

Merged
merged 2 commits into from
May 22, 2019

Conversation

LukasPaczos
Copy link
Member

Closes #14048.

This PR adds a possibility to listen to thread's lifecycle within the thread pool and changes the Android's shared thread pool implementation to attach each of the threads in the pool to the JVM. This way, we are not constantly attaching/detaching JVM to those threads which was forcing GC runs.

@LukasPaczos LukasPaczos added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Apr 17, 2019
@LukasPaczos
Copy link
Member Author

Comment addressed. Just noting that I still need to make sure that https://circleci.com/gh/mapbox/mapbox-gl-native/264818 was not introduced by this PR.

@LukasPaczos
Copy link
Member Author

I've run CI jobs multiple times and couldn't catch the issue from #14450 (comment), neither could I reproduce locally. I think it's safe to say that it was a one-time occurrence, but I will monitor it further.

Anyway, this is ready for another round.

@LukasPaczos LukasPaczos force-pushed the lp-thread-detaching-14048 branch 3 times, most recently from cde3f5f to 3938e16 Compare April 30, 2019 11:47
@LukasPaczos LukasPaczos added this to the release-n milestone Apr 30, 2019
@tmpsantos
Copy link
Contributor

@LukasPaczos can we hold on this until we land #13935 ?

@LukasPaczos
Copy link
Member Author

Sure, what's the timeline for #13935 @tmpsantos?

@tmpsantos
Copy link
Contributor

Sure, what's the timeline for #13935 @tmpsantos?

2 weeks

@LukasPaczos LukasPaczos force-pushed the lp-thread-detaching-14048 branch 2 times, most recently from 6299e3c to 4da712a Compare May 10, 2019 15:08
@LukasPaczos
Copy link
Member Author

Thanks a lot for all the feedback! Since #13935 landed, I've changed the PR a little bit and it's ready for another round.

@LukasPaczos LukasPaczos force-pushed the lp-thread-detaching-14048 branch 3 times, most recently from a50c56a to 49dd3eb Compare May 10, 2019 15:38
@LukasPaczos LukasPaczos requested a review from 1ec5 as a code owner May 10, 2019 15:38
@LukasPaczos LukasPaczos requested a review from a team May 10, 2019 15:38
@LukasPaczos LukasPaczos removed the request for review from a team May 21, 2019 10:48
@LukasPaczos LukasPaczos requested review from tmpsantos and removed request for 1ec5 May 21, 2019 10:48
@LukasPaczos
Copy link
Member Author

I've re-written to PR to follow @tmpsantos suggestion to use platform::. While the previously proposed TheadLifecycle might be more versatile, there'd be no use in this versatility as of now, therefore a simpler solution is definitely better.

include/mbgl/util/platform.hpp Outdated Show resolved Hide resolved
platform/android/src/thread.cpp Outdated Show resolved Hide resolved
@LukasPaczos LukasPaczos force-pushed the lp-thread-detaching-14048 branch 3 times, most recently from b7b0724 to 74bf702 Compare May 22, 2019 12:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frequent GeoJSONSource updates force GC every couple seconds
5 participants