[core] Modularize FileSource codebase #15768

alexshalamov · 2019-10-07T07:34:37Z

FileSource modularization

At the moment, gl-native has monolithic FileSource codebase that suffers from following issues:

Difficult to disable features (sources) that are not required
Difficult to plug-in FileSource implementation and override default backends (offline, local file, asset file sources)
Base interfaces expose implementation details (threading, testing APIs, actor model)
FileSource naming is confusing, as we have many SomethingSource classes and most of them have nothing to do with FileSource code

This PR aims to modularize FileSource codebase and introduces following changes:

Adds FileSourceManager interface that provides access to FileSource instances and means of registering / unregistering FileSource factories
Splits DefaultFileSource into smaller parts
Adds DatabaseFileSource interface and it's default implementation
Removes inter-dependencies between concrete FileSource classes
All sources operate on dedicated thread, except MainResourceLoader that acts as a dispatcher and works on thread that requested it.

TODO:

Identified improvements for follow-up PRs

Add batch resource request API
Set 'accessed' flag for requested database resources in batches, under transaction
Don't use #include <mbgl/storage/sqlite3.hpp> headers in Android FileSource impl
Split Android FileSource impl
Remove usage of "online" functionality from MGLOfflineStorage and split it to separate objects
Mass-rename FileSource and other related classes to ResourceProvider to avoid confusion with GeoJSONSource, RasterSource, VectorSource, CustomGeometrySource and other *Sources. AssetResourceProvider, FileSystemResourceProvider, NetworkResourceProvider...
Add build flags, so that gl-native can be built without features that are not required, for instance: MBGL_WITH_FILESYSTEM_RESOURCE_PROVIDER, MBGL_WITH_DATABASE_RESOURCE_PROVIDER etc.
Split Database related code into two interfaces, one for caching and one for offline use-cases
Threading improvements: Create threads (task runners) dedicated to IO, Networking. Would be easier to run blocking code on these threads if needed. Similar to Chromium's base::PostTask*

alexshalamov · 2019-10-18T07:15:47Z

@tobrun @LukasPaczos could you please take a look at Android part : bb2c870

@julianrex @fabian-guerra similarly, could you check darwin related changes: d8eb4e7

pozdnyakov

Further, we can reconsider using actor

include/mbgl/storage/database_file_source.hpp

include/mbgl/storage/file_source.hpp

pozdnyakov · 2020-01-07T14:27:17Z

include/mbgl/storage/database_file_source.hpp

-// TODO: the callback should include a potential error info when https://github.com/mapbox/mapbox-gl-native/issues/14759 is resolved
-using PathChangeCallback = std::function<void ()>;
+    // FileSource overrides
+    std::unique_ptr<AsyncRequest> request(const Resource&, Callback) override;


Callback I'd also substitute with the aliased type

platform/default/src/mbgl/storage/database_file_source.cpp

tmpsantos · 2020-01-08T12:20:36Z

platform/default/src/mbgl/storage/file_source_manager.cpp

+
+FileSourceManager* FileSourceManager::get() noexcept {
+    static DefaultFileSourceManagerImpl instance;
+    return &instance;


If this is non-nullable, why not return a reference insteaf of a pointer?

Two reasons:

API style. Layer manager uses same pattern

Map don't even need file source manager, for example, when fully inlined style is used (map.cpp)
FileSourceManager::get() ? FileSourceManager::get()->getFileSource(ResourceLoader, resourceOptions) : nullptr,

tmpsantos · 2020-01-08T12:23:56Z

platform/default/src/mbgl/storage/file_source_manager.cpp

+
+namespace mbgl {
+
+class DefaultFileSourceManagerImpl final : public FileSourceManager {


Only issue I see with this is it is a linking time dependency, meaning that if you don't implement FileSourceManager::get() it will not link. Would't be better if the platform could simple register the factories or not explicitly during initialization?

I'm fine with keeping like this btw and migrate everything later.

platform/default/src/mbgl/storage/main_resource_loader.cpp

src/mbgl/tile/tile_loader_impl.hpp

include/mbgl/storage/file_source.hpp

* [core] Introduce FileSourceManager and use it for default platform impl - Add `FileSourceManager` interface that provides access to `FileSource` instances and means of registering / unregistering `FileSource` factories - Split `DefaultFileSource` into smaller parts - Add `DatabaseFileSource` interface and it's default implementation - Remove inter-dependencies between concrete `FileSource` classes * [build] Add files to next build system * [core] Add generic property setters / getters * [core] Remove setOnlineStatus from OnlineFileSource interface * [core] Hide threading implementation details from DatabaseFileSource interface * [core] Make DB file source methods virtual * [core] Add documentation for DatabaseFileSource and rename one method * [core] Use simple callback instead of ActorRef * [core] Remove ActorRef from OnlineFileSource public header * [core] Add callback to FileSource::forward async API * [core] Pass OfflineRegionDefinition by value * [core] Update tests to use modular file sources * [core] Update unit tests * [core] Update unit tests after rebase * [core] Backport low prio fix for cached requests * [core] Backport pack database * [core] Return removed factory from unRegisterFileSourceFactory * [core] Rename shadowed args in onlinefilesource * [core] Remove simple std::function callback aliases * [core] Expose online file source property keys in public header file * [test-runner] Add proxy file source test runner * [cache] Update mbgl-cache utility to use new file source * [metrics] Rebaseline binary size metrics * [offline] Update offline utility * [core] Update changelog

Use new interface for android jni adaptation classes.

* [core] Introduce FileSourceManager and use it for default platform impl - Add `FileSourceManager` interface that provides access to `FileSource` instances and means of registering / unregistering `FileSource` factories - Split `DefaultFileSource` into smaller parts - Add `DatabaseFileSource` interface and it's default implementation - Remove inter-dependencies between concrete `FileSource` classes * [build] Add files to next build system * [core] Add generic property setters / getters * [core] Remove setOnlineStatus from OnlineFileSource interface * [core] Hide threading implementation details from DatabaseFileSource interface * [core] Make DB file source methods virtual * [core] Add documentation for DatabaseFileSource and rename one method * [core] Use simple callback instead of ActorRef * [core] Remove ActorRef from OnlineFileSource public header * [core] Add callback to FileSource::forward async API * [core] Pass OfflineRegionDefinition by value * [core] Update tests to use modular file sources * [core] Update unit tests * [core] Update unit tests after rebase * [core] Backport low prio fix for cached requests * [core] Backport pack database * [core] Return removed factory from unRegisterFileSourceFactory * [core] Rename shadowed args in onlinefilesource * [core] Remove simple std::function callback aliases * [core] Expose online file source property keys in public header file * [test-runner] Add proxy file source test runner * [cache] Update mbgl-cache utility to use new file source * [metrics] Rebaseline binary size metrics * [offline] Update offline utility * [core] Update changelog

This reverts commit 879c44f.

alexshalamov self-assigned this Oct 7, 2019

alexshalamov force-pushed the alexshalamov_modular_file_source branch 21 times, most recently from 04ca71c to 2cace7a Compare October 11, 2019 15:55

alexshalamov changed the title ~~WIP: [core] Modularize FileSource codebase~~ [core] Modularize FileSource codebase Oct 12, 2019

alexshalamov force-pushed the alexshalamov_modular_file_source branch 2 times, most recently from 0a7262a to 07c6b5d Compare October 14, 2019 07:56

alexshalamov marked this pull request as ready for review October 14, 2019 13:40

alexshalamov requested a review from 1ec5 as a code owner October 14, 2019 13:40

alexshalamov requested review from a team, tmpsantos and pozdnyakov October 14, 2019 13:40

alexshalamov requested a review from tobrun October 14, 2019 17:55

chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Oct 15, 2019

chloekraw added this to the release-unicorn milestone Dec 18, 2019

alexshalamov force-pushed the alexshalamov_modular_file_source branch 4 times, most recently from c6887a8 to 84e2f00 Compare January 6, 2020 11:34

pozdnyakov approved these changes Jan 7, 2020

View reviewed changes

tmpsantos approved these changes Jan 8, 2020

View reviewed changes

alexshalamov force-pushed the alexshalamov_modular_file_source branch 2 times, most recently from bdd3f7c to 315d917 Compare January 9, 2020 15:48

alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Jan 9, 2020

alexshalamov force-pushed the alexshalamov_modular_file_source branch from 315d917 to b208892 Compare January 10, 2020 17:19

alexshalamov added 5 commits January 10, 2020 23:17

[node] Use new FileSourceManager interface

2f4b352

[darwin] Use new FileSourceManager interface

ec2a256

[qt] Use new FileSourceManager interface

aa8738a

[android] Use new FileSourceManager interface

e1ed063

Use new interface for android jni adaptation classes.

alexshalamov force-pushed the alexshalamov_modular_file_source branch from b208892 to e1ed063 Compare January 10, 2020 21:18

alexshalamov merged commit 6ddff19 into master Jan 13, 2020

alexshalamov deleted the alexshalamov_modular_file_source branch January 13, 2020 08:57

julianrex mentioned this pull request Jan 22, 2020

Updates mapbox-gl-native submodule to release-unicorn branch mapbox/mapbox-gl-native-ios#151

Merged

alexshalamov added a commit that referenced this pull request Jan 31, 2020

Revert "[core] Modularize FileSource codebase (#15768)"

6479d0d

This reverts commit 879c44f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Modularize FileSource codebase #15768

[core] Modularize FileSource codebase #15768

alexshalamov commented Oct 7, 2019 •

edited

Loading

alexshalamov commented Oct 18, 2019

pozdnyakov left a comment

pozdnyakov Jan 7, 2020

tmpsantos Jan 8, 2020

alexshalamov Jan 10, 2020

tmpsantos Jan 8, 2020

tmpsantos Jan 8, 2020


		namespace mbgl {

		class DefaultFileSourceManagerImpl final : public FileSourceManager {

[core] Modularize FileSource codebase #15768

[core] Modularize FileSource codebase #15768

Conversation

alexshalamov commented Oct 7, 2019 • edited Loading

FileSource modularization

TODO:

Identified improvements for follow-up PRs

alexshalamov commented Oct 18, 2019

pozdnyakov left a comment

Choose a reason for hiding this comment

pozdnyakov Jan 7, 2020

Choose a reason for hiding this comment

tmpsantos Jan 8, 2020

Choose a reason for hiding this comment

alexshalamov Jan 10, 2020

Choose a reason for hiding this comment

tmpsantos Jan 8, 2020

Choose a reason for hiding this comment

tmpsantos Jan 8, 2020

Choose a reason for hiding this comment

alexshalamov commented Oct 7, 2019 •

edited

Loading