Skip to content

Reinitialize UsedObjects when enabling#897

Open
Symmetricity wants to merge 1 commit into
systemed:masterfrom
Symmetricity:fix/reinitialize-used-objects
Open

Reinitialize UsedObjects when enabling#897
Symmetricity wants to merge 1 commit into
systemed:masterfrom
Symmetricity:fix/reinitialize-used-objects

Conversation

@Symmetricity
Copy link
Copy Markdown

This PR is AI generated.

Summary

This reinitializes UsedObjects storage when it is enabled after being cleared.

UsedObjects::clear() frees the node-tracking storage after the nodes phase to
reduce memory use. When tilemaker reads multiple input PBFs in one process, the
same OSMStore instance can then enable usedNodes again for the next input.
UsedObjects::set() and UsedObjects::test() both assume the outer chunk table
exists, so re-enabling after clear() can otherwise leave those methods
indexing an empty vector.

Background

usedNodes is disabled by default and is only enabled when the Lua profile
declares way_keys.

The read lifecycle is:

  1. PbfProcessor::ReadPbfFile() calls osmStore.usedNodes.enable() before
    WayScan when way_keys are enabled.
  2. ScanRelations() and ScanWays() can populate usedNodes.
  3. ReadNodes() tests usedNodes to decide which nodes to keep.
  4. After ReadPhase::Nodes, osmStore.usedNodes.clear() frees the storage.
  5. If another PBF input is read by the same tilemaker invocation, step 1 can
    happen again on the same UsedObjects instance.

Before this patch, step 5 only set the status back to enabled. It did not
restore the outer chunk table that clear() had removed.

Implementation

The patch is intentionally small:

  • UsedObjects::enable() recreates the outer chunk table if it was previously
    cleared.
  • UsedObjects::clear() swaps with an empty vector so the outer table capacity
    is released rather than retained.

The hot set() and test() paths are unchanged.

Why This Location

The reinitialization belongs in enable() because that is the lifecycle
boundary where a previously cleared UsedObjects becomes active again.

Adding recovery checks to set() or test() would also avoid the crash, but
those are hot paths used while parsing. Keeping the check in enable() makes
the fix cheaper and keeps the normal lookup/update paths unchanged.

clear() remains the right place to release the memory because it is called
after ReadPhase::Nodes, once the node-tracking table is no longer needed for
that input.

Possible Regressions

The intended behavior change is limited to multiple-PBF runs where usedNodes
is enabled again after being cleared.

Expected behavior:

  • single-PBF runs are unchanged
  • disabled UsedObjects behavior is unchanged
  • set() and test() semantics are unchanged once enabled
  • memory used by the outer chunk table is allocated again only if the object is
    re-enabled

The patch does not address broader multi-PBF state questions such as whether
relation-scan state should be reset or retained between inputs. It only fixes
the UsedObjects lifecycle after its storage has been deliberately cleared.

Testing

Recommended checks:

git diff --check origin/master..fix/reinitialize-used-objects
cmake -S . -B local-builds/reinitialize-used-objects -DCMAKE_BUILD_TYPE=RelWithDebInfo
cmake --build local-builds/reinitialize-used-objects --target tilemaker -j2

Manual reproduction should use at least two input PBFs with an OpenMapTiles-like
profile that declares way_keys, so usedNodes is enabled for the first input,
cleared after its nodes phase, and then enabled again for the second input.

UsedObjects::clear frees its node tracking storage after the nodes phase.

When reading multiple PBF inputs, the same OSMStore can enable usedNodes again for the next input. set() and test() expect the outer chunk table to exist, so recreate that table when enabling if it was previously cleared.

Swap with an empty vector when clearing so the outer chunk table allocation is released rather than retained as capacity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant