-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
Ambient cache initialization speed improvement #1959
base: main
Are you sure you want to change the base?
Ambient cache initialization speed improvement #1959
Conversation
This reverts commit 07f7fa4.
…Resource / putInternal and getTile / getResource which support that Fix for NOT NULL contraint violation for "compressed" field when inserting tile or resource Changed SQL query for ambient cache initialization
…iles table when resource / tile is needed in an offline region download
…s tables (added to the upgrade script) + queries modified to manage LRU based on new tables
…n yet at the moment but it is working
Fixed existing tests to comply with new database model (work in progress) + temporarily disabled some database merging tests which crashed + added new MigrateFromV6Schema test method
added v6.db in test/fixtures/offline_database
…deleted after deleting an offline region
…ew database model version (v7)
…w database model version (v7)
for more information, see https://pre-commit.ci
The OfflineDatabase.CorruptDatabaseOnQuery still fails. We would need to create a corrupted database in version 7 of the database model, which opens without any error, but where the first query triggers error of corruption (with test database in version 6, simply instantiating the object triggers the corruption error when it tries to create the new tables for the model upgrade, which makes the test fail) |
How did you make the databases? |
For databases used for merging / sideloading tests (satellite_test.db, sideload_ambient.db, sideload_sat.db, and sideload_sat_multiple.db), I simply upgraded them with a SQLite database tool by running the upgrade script to model version 7. For v6.db database (which is used to test the upgrade from version 6 to version 7), I made a copy of v5.db and upgraded it with a SQLite database tool by running the upgrade script to model version 6. I still haven't managed to create the upgraded corrupt-delayed.db database (a corrupted database in version 7 of the database model, which opens without any error, but where the first query triggers error of corruption) to make the OfflineDatabase.CorruptDatabaseOnQuery test succeed :-( |
Bloaty Results (iOS) 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-1959-compared-to-main.txt |
CorruptDatabaseOnQuery test temporarily disabled
for more information, see https://pre-commit.ci
CorruptDatabaseOnQuery test has been temporarily disabled as discussed with @louwers |
Bloaty Results 🐋Compared to main
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-1959-compared-to-main.txtCompared to d387090 (legacy)
Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-1959-compared-to-legacy.txt |
"SELECT length(data) FROM ambient_tiles WHERE url_template = ?1 AND pixel_ratio = ?2 AND x = ?3 AND y = ?4 " | ||
"AND z = ?5"); | ||
if (selectAmbientTilesResult) { | ||
// std::cout << "-------- HASTILE - FOUND IN AMBIENT_TILES\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are commented out lines that print used for debugging throughout this file.
Please remove them or change them to (debug) logging.
I ran the
No significant difference interestingly. |
Removed logs in comments
…ves/maplibre-native into fix/ambient-cache-slowness
Hello @louwers I have just removed the cout logs in comments. About the performance test, the improvements are significant when there is a lot of offline data downloaded as offline regions (for example 1,5 GB). The startup of the app is much faster in that case :-) |
for more information, see https://pre-commit.ci
const Response& response, | ||
const std::string& data, | ||
bool compressed) { | ||
bool OfflineDatabase::putResource( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is too much code duplication in this function and the putTile
function below.
Instead of adding a bool
I would for example define an enum class
that can be converted to a string
to get the related database name using another utility function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agree with you about the code duplication issue, but we encountered difficulties in resolving it :-(
Initially, we attempted to minimize code duplication in methods like getTile(), getResource(), putTile(), putResource(), etc., by using lambdas and c_str() to concatenate the table name in the query. However, this approach led to memory issues (EXC_BAD_ACCESS) and erratic behavior, particularly errors during the insertion of tiles/resources, for example :
[DEBUG] {abort at 23 in [INSERT INTO ambient_tiles (url_template, pixel_ratio, x, y, z, modified, must_revalidate, etag, expires, accessed, data, compressed) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)]: NOT NULL constraint failed: ambient_tiles.compressed}Database: %s
Consequently, we had to revert our changes, even though it resulted in increased code duplication.
You can find here an old version of the code here with our trials for getTile() and getResource() method: Geolives@cb8cf11
As our expertise in C++ is not as strong as it is in other higher-level languages, we welcome any assistance from the community to help resolve this issue of code duplication without causing adverse effects :-)
Thanks in advance.
I downloaded 4GB of offline data. This is a build before this PR. It loads pretty quickly...? https://youtube.com/shorts/CZqLHsIXg4I?si=abdOzSF47ZvxOeJ5 This is a build that includes this PR: https://youtube.com/shorts/ocZza9EIdzk?si=9-9kGuG6XKy4xnXd I guess that means that |
@@ -35,6 +35,35 @@ static constexpr const char* offlineDatabaseSchema = | |||
" must_revalidate INTEGER NOT NULL DEFAULT 0,\n" | |||
" UNIQUE (url_template, pixel_ratio, z, x, y)\n" | |||
");\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is generated and should not be modified manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try updating offline_schema.js
and running that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I didn't know that the file was autogenerated, sorry, I will try to do that :-)
// First, try to find the tile in the 'tiles' table | ||
std::optional<int64_t> selectTilesResult = extractTileDataSize( | ||
tile, | ||
"SELECT length(data) FROM tiles WHERE url_template = ?1 AND pixel_ratio = ?2 AND x = ?3 AND y = ?4 AND z = ?5"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always fail for users not using any offline functionality at all. I'm not comfortable proceeding without first ensuring the performance impact is minimal, otherwise we need to keep track of if the tiles
table contains any tiles at all and skip this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agree with you on this point, a boolean variable which would be initialized on startup (by counting tiles / resources, or even by checking if there is an offline_region in the database or not) and set when we add the first resource / tile is a good idea in our opinion, we will try to implement that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again apologies for taking so long for me to review this, but I wasn't at all familiar with this subsystem of the library.
It looks like the problematic initAmbientCacheSize
method was only introduced so that cache size could be determined without taking into account offline data. mapbox/mapbox-gl-native#15622
It seems these changed can be reverted after the split?
By the way, I was not able to reproduce the performance problems. Are you calling setMaximumAmbientCacheSize
on startup? It seems somehow I was able to store 3GB of data with OfflineActivity.kt
, without callingsetMaximumAmbientCacheSize
and without an evict
being triggered (these are the only places that call initAmbientCacheSize
).
In our application, the problem was much more reproducible on iOS than on Android (the reason for this is unclear). In the iOS test application integrated into the maplibre-native project, the problem could be reproduced as explained here: #1815, and as you can see in this video: https://github.com/maplibre/maplibre-native/assets/13694294/a7c2bf01-54ff-43ed-86b1-675efb79e00a. On Android, the performance issue was significantly less pronounced. |
@Geolives Alright, I will try iOS as well. What do you think about removing |
It requires further investigation because we still need to compute the ambient cache size to avoid exceeding the maximum chosen size. However, you are right that we need to understand how it was done before integrating mapbox/mapbox-gl-native#15622 |
In order to solve #1815
Here is the matching design proposal : #1877