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

[core] Make the BackgroundScheduler a singleton #13935

Merged
merged 2 commits into from
May 10, 2019
Merged

[core] Make the BackgroundScheduler a singleton #13935

merged 2 commits into from
May 10, 2019

Conversation

tmpsantos
Copy link
Contributor

@tmpsantos tmpsantos commented Feb 15, 2019 •
edited
Loading

  • Do not carry it over everywhere as parameter, it is a shared
    instance anyway and the lifecycle is pretty much the app lifecycle
    from the moment we instantiate a map.
  • Rename to BackgroundScheduler because it is a Scheduler that will
    do tasks in the background, we don't make assumptions if it is a
    thread pool or a single thread.
  • Most importantly, remove the dependency from core on platform.

@tmpsantos tmpsantos requested a review from 1ec5 as a code owner February 15, 2019 14:28
@tmpsantos tmpsantos requested a review from a team February 15, 2019 14:28
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch from 4c12d6b to c19934d Compare February 15, 2019 14:29
@tmpsantos tmpsantos requested a review from pozdnyakov February 15, 2019 14:30
julianrex
julianrex reviewed Feb 15, 2019
View reviewed changes
Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

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

Do you have concerns about using platform::GetBackgroundScheduler() directly, rather than being injected? Seems that it more tightly couples those classes.

Could this also affect the tests? We've got a similar thing on iOS where parallel tests are affecting each other due to a shared singleton (and I'm trying to remove the dependency on the singleton in that instance!)

platform/default/src/mbgl/platform/thread_pool.cpp Outdated Show resolved Hide resolved
platform/node/src/node_thread_pool.cpp Outdated Show resolved Hide resolved
@pozdnyakov
Copy link
Contributor

Do you have concerns about using platform::GetBackgroundScheduler() directly, rather than being injected? Seems that it more tightly couples those classes.

True, however IMO background worker is generic enough to be considered as a part of the project "foundation classes" :)

@tmpsantos tmpsantos self-assigned this Feb 22, 2019
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch 2 times, most recently from b2d93c9 to 31dd705 Compare February 25, 2019 14:33
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch 5 times, most recently from 657bd0e to c3e201c Compare March 11, 2019 22:07
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch from c3e201c to 3011269 Compare March 12, 2019 14:29
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch 5 times, most recently from 1490fb0 to dc4195e Compare April 11, 2019 12:09
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch 7 times, most recently from 444276d to 480de64 Compare May 3, 2019 13:57
@tmpsantos tmpsantos mentioned this pull request May 6, 2019
Merged
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch from 480de64 to 02b821d Compare May 7, 2019 17:12
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch 2 times, most recently from d7b108c to 4403058 Compare May 8, 2019 10:29
pozdnyakov
pozdnyakov reviewed May 8, 2019
View reviewed changes
include/mbgl/actor/actor.hpp Outdated Show resolved Hide resolved
include/mbgl/actor/scheduler.hpp Show resolved Hide resolved
platform/default/src/mbgl/util/run_loop.cpp Outdated Show resolved Hide resolved
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch 3 times, most recently from 06718e8 to 1f35089 Compare May 9, 2019 13:05
alexshalamov
alexshalamov approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@alexshalamov alexshalamov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nit: remove benchmark/fixtures/api/cache.db from the PR, maybe add it to .gitignore?
Also, need to be synchronized with #14620, whichever lands first.

include/mbgl/actor/scheduler.hpp Show resolved Hide resolved
tmpsantos added 2 commits May 10, 2019 11:26
@tmpsantos
[core] Make the BackgroundScheduler a singleton
ea8943e 
- Do not carry it over everywhere as parameter, it is a shared
  instance anyway and the lifecycle is pretty much the app lifecycle
  from the moment we instantiate a map.
- Rename to BackgroundScheduler because it is a Scheduler that will
  do tasks in the background, we don't make assumptions if it is a
  thread pool or a single thread.
- Most importantly, remove the dependency from `core` on `platform`.
@tmpsantos
[node] Remove NodeThreadPool
5c355fd 
Simplify, use the default thread pool, less platform abstraction.
@tmpsantos tmpsantos force-pushed the tmpsantos-thread_pool branch from 1f35089 to 5c355fd Compare May 10, 2019 08:26
@tmpsantos tmpsantos merged commit cf08f64 into master May 10, 2019
@tmpsantos tmpsantos deleted the tmpsantos-thread_pool branch May 10, 2019 11:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@julianrex julianrex julianrex left review comments

@alexshalamov alexshalamov alexshalamov approved these changes

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@1ec5 1ec5 Awaiting requested review from 1ec5

Assignees

@tmpsantos tmpsantos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmpsantos @pozdnyakov @alexshalamov @julianrex