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.

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

Reduce memory consumption from attribute accumulation #290

Merged
merged 8 commits into from
Nov 5, 2024
Merged

Conversation

e-n-f
Copy link
Collaborator

@e-n-f e-n-f commented Nov 4, 2024

The externally-visible effect is that -zg maxzoom guessing now takes the number of features with duplicate locations into account. The main calculation is still the same, and only takes into account distinct locations, but the base zoom is then increased if necessary to ensure that the number of features present in each zoom level, distinctly visible or not, is reasonable.

The internal change is that attribute names in serial_feature are now stored as shared pointers, so attribute names can be shared between features instead of having to be stored individually.

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.

Notes from review:

Generally looks good -- we're trading a certain drop in memory usage from the two optimizations against a few new uncertainties:

  • Maybe tiling takes a lot longer from going to higher zooms in some cases
  • Maybe low zooms are changed too much in those datasets with lots of duplicates from having chosen to go to higher zooms?
  • Maybe running time of tiling is longer because of extra indirection with shared_ptr optimization, plus costs of looking up keys in the map?

But all of those unknowns have at least some coverage in our existing tests, so we're OK with the fact that there might be some places where they're a net loss.

But before merging, let's add at least one automated test case for this duplicate -> higher zoom pathway.

@e-n-f e-n-f marked this pull request as ready for review November 5, 2024 22:11
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.

Thanks for the test!

@e-n-f e-n-f merged commit 23667bb into main Nov 5, 2024
4 checks passed
@e-n-f e-n-f deleted the accumulate-mem branch November 5, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants