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

Eliminate copies in deferred cleanup #3035

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

TimSylvester
Copy link
Collaborator

Pulls in an implementation of move_only_function.

This simplifies deferred deletions, since there's no temporary copy to potentially outlast the schedule call, and eliminates the need to convert unique_ptrs to shared_ptrs.

Unfortunately, the change of function types for the scheduler spreads to the map callbacks due to usage like:

pathChangeCallback = Scheduler::GetCurrent()->bindOnce(...

This is further complicated by the fact that of the callbacks are explicitly copied:

void FileSourceRequest::setResponse(const Response& response) {
    // Copy, because calling the callback will sometimes self
    // destroy this object. We cannot move because this method
    // can be called more than once.

Also fixes various style warnings.

Working in Android and ios so far.

Is this worth pursuing further?

[18341ea] use public dep (+12 squashed commits)
[8ce96e4] fix glfw/xcode build
[0511a86] cherry-pick 366ebcc
[5b8a962] Disable static downcast warnings
[c7f4dea] improve type reference
[90c6a0b] xcode build
[8642002] use `std23::move_only_function` (+6 squashed commits)
[e57500aa58] improve coverage, rebase on main
[e98eab5cb7] normalize syntax
[d8dc4777ca] back out capture changes
[29610f6115] clear vector to resume a well-defined state
[4ff6f3a4e0] release pending items before waiting
[27d1cfacae] Set deferred deletions aside and schedule them once per frame (per source)
Copy link

github-actions bot commented Jan 10, 2025

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.7% -91.0Ki  -0.5% -64.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-3035-compared-to-main.txt

Copy link

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1%  +115Ki  +0.0%     +40    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3035-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +31% +36.2Mi  +438% +26.2Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3035-compared-to-legacy.txt

Copy link

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0055         +0.0052             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-3035-compared-to-main.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant