Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pull from the back of the list of tiles to overzoom, not the front #149

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Pull from the back of the list of tiles to overzoom, not the front #149

merged 1 commit into from
Oct 4, 2023

Conversation

e-n-f
Copy link
Collaborator

@e-n-f e-n-f commented Oct 4, 2023 •
edited
Loading

This makes a surprisingly large improvement to speed.

Before:

(.venv) Erica-Felt% ./tippecanoe --version                                                                            
tippecanoe v2.34.0
(.venv) Erica-Felt% time ./tile-join --overzoom -z16 -f -o foo.mbtiles  50707ddf-fd34-5d1f-a855-7eedba0b8e00/pmtiles/*
./tile-join --overzoom -z16 -f -o foo.mbtiles   82.53s user 4.60s system 70% cpu 2:04.14 total

After:

(.venv) Erica-Felt% ./tippecanoe --version                                                                            
tippecanoe v2.34.1
(.venv) Erica-Felt% time ./tile-join --overzoom -z16 -f -o foo.mbtiles  50707ddf-fd34-5d1f-a855-7eedba0b8e00/pmtiles/*
./tile-join --overzoom -z16 -f -o foo.mbtiles   0.90s user 0.23s system 99% cpu 1.146 total

(50707ddf is microcuencas.zip)

@e-n-f e-n-f marked this pull request as ready for review October 4, 2023 20:45
e-n-f
e-n-f commented Oct 4, 2023
View reviewed changes
tile-join.cpp
@@ -1061,7 +1062,7 @@ void decode(struct tileset_reader *readers, std::map<std::string, layermap_entry
// Then this tile is done and we can safely run the output queue.

if (readers == NULL || readers->zoom != current.first.z || readers->x != current.first.x || readers->y != current.first.y) {
if (tasks.size() > 10 * CPUS) {
if (tasks.size() > 100 * CPUS) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change from 100 to 10 in the last PR was accidental, left over from testing whether that would reduce memory usage. Restoring it to 100 does not make a big difference to total running time but does reduce time spent on I/O.

ChrisLoer
ChrisLoer approved these changes Oct 4, 2023
View reviewed changes
Copy link

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

Approved after talking through at https://feltmaps.slack.com/archives/C02SJ3UEF3R/p1696452376499689

@e-n-f e-n-f merged commit 0ca1408 into main Oct 4, 2023
1 check passed
@e-n-f e-n-f deleted the tile-join-overzoom-queue branch October 4, 2023 21:59
@e-n-f e-n-f mentioned this pull request Oct 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisLoer ChrisLoer ChrisLoer approved these changes

@joeherm joeherm Awaiting requested review from joeherm

@migurski migurski Awaiting requested review from migurski migurski is a code owner

@arredond arredond Awaiting requested review from arredond arredond is a code owner

@dnomadb dnomadb Awaiting requested review from dnomadb dnomadb is a code owner

@ibesora ibesora Awaiting requested review from ibesora

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@e-n-f @ChrisLoer