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

[core] Calculate size of an ambient cache without offline region's resources #15622

Merged
merged 4 commits into from
Mar 7, 2020

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Sep 12, 2019

Resources that belong to an offline region, should not contribute to the amount of space available in the ambient cache.

Fixes: #4411
Fixes: https://github.com/mapbox/mapbox-gl-native-team/issues/212

@alexshalamov alexshalamov self-assigned this Sep 12, 2019
@alexshalamov alexshalamov marked this pull request as ready for review September 12, 2019 20:35
@alexshalamov
Copy link
Contributor Author

@tobrun @julianrex Documentation for darwin and Android has to be updated. Do you want to do it yourself in separate PR?

Also, this is an API break, since the old behavior, is changed, and some apps may have a workarounds that resizes ambient cache every-time offline pack is downloaded. Not sure if worth making semver release.

Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

few nits

platform/default/src/mbgl/storage/offline_database.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/offline_database.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/offline_database.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/offline_database.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@tmpsantos tmpsantos left a comment

Choose a reason for hiding this comment

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

The only concern I have about this approach is the ambient cache size reduction is precisely measure using the delta of number of pages but the growth is taking into consideration only the data size.

Meaning that if we add and remove the same entry, the added size is always going to be smaller than the removed size, and in the long run the cache size will be zero.

@alexshalamov
Copy link
Contributor Author

@tmpsantos Changed code a bit to get more precise ambient cache growth size, ptal.

@julianrex julianrex added Core The cross-platform C++ core, aka mbgl needs discussion ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Sep 16, 2019
@chloekraw
Copy link
Contributor

Also, this is an API break, since the old behavior, is changed, and some apps may have a workarounds that resizes ambient cache every-time offline pack is downloaded. Not sure if worth making semver release.

In my opinion, if we flag this change clearly at the top of the changelog under a "Major improvements" section, this would be acceptable in a minor release. cc @mapbox/maps-android @mapbox/maps-ios

@alexshalamov could you draft a changelog entry and I'll think about it and review? Thanks for picking this longstanding problem up!

@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Oct 15, 2019
@pozdnyakov pozdnyakov removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Oct 31, 2019
@alexshalamov alexshalamov force-pushed the alexshalamov_ambient_cache_size branch from fc7139e to 22a250d Compare March 6, 2020 12:29
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 6, 2020
@alexshalamov alexshalamov force-pushed the alexshalamov_ambient_cache_size branch 2 times, most recently from 14d4deb to c553b07 Compare March 6, 2020 12:33
@alexshalamov
Copy link
Contributor Author

alexshalamov commented Mar 6, 2020

@julianrex @1ec5 @tobrun we are planning to release this fix in a next release (maps-v1.4.0)

Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

lgtm % nits

@alexshalamov alexshalamov force-pushed the alexshalamov_ambient_cache_size branch from c1afd2e to 4ee189b Compare March 6, 2020 15:00
@alexshalamov alexshalamov force-pushed the alexshalamov_ambient_cache_size branch from 4ee189b to dae65f6 Compare March 6, 2020 22:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ambient caching disabled when the database contains 50 MB+ of offline resources
5 participants