Skip to content

NMP speed improvements - #508

Merged
jteresco merged 2 commits into
TravelMapping:masterfrom
yakra:nmp_split
Mar 25, 2022
Merged

NMP speed improvements#508
jteresco merged 2 commits into
TravelMapping:masterfrom
yakra:nmp_split

Conversation

@yakra

@yakrayakra commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

C++

Two components here:

  • CloseWaypointQuadtree::near_miss_waypoints yakra/DataProcessing#157
    Don't create a bunch of temporaries and muck about with move constructors & splicing; just push points directly to the nmp list in the Waypoint object itself. Saves a rather surprising amount of time for what it is.
    nmpdirect
  • CloseSingle-threaded NMP detection: restore Jim's original formula #457
    When no-mutex NMPs #280 were enabled, multi-threaded NMP searching came at the cost of a performance hit in single-threaded searches. Background is at idea: replace per-point NMP lists with a method #141.
    Conditioning Jim's original formula back in for single-threaded siteupdateST is easy.
    Speed comparison after the above component, constructing in place:
    • Time no longer spent specifically Searching for near-miss points via Eric's method should outweigh
    • the additional time Reading waypoints for all routes, which represents Jim's method.
      BoxEric’sJim’s
      BT1.721.437
      lab11.0090.6198
      lab51.4351.033
      lab21.3490.966
      lab31.791.092
      lab42.161.601
      bsdlab2.5811.752
  • A third component I tried out & rejected was to
    { // sort the near miss points for consistent ordering to facilitate NMP FP marking
    in NmpSearchThread rather than a single-threaded part of the program. Turns out this is slower.
    What can I come up with for an explanation?
    • Turning cache hits into cache misses -- the single-threaded bit already deals with root & label.
    • Branch prediction effects, whether sorting is done at the same time as iterating down through the quadtree.

Python?

Python OTOH has kept Jim's original "search a partially populated quadtree while reading all waypoints" NMP method the whole time. The only thing to do here is to try out building the NMP lists directly in the Waypoint object.
We're still creating and copying (not moving, I presume?) from a bunch of temporaries, right? I'd expect better results than:

Boxde796faa768800
epoch115.24120.19
BT113.3111.59
lab156.9857.07
lab551.1851
lab272.2472.09
lab398.1597.2
lab480.2679.61
bsdlab126.58125.06
noreaster93.4692.53

Python is pretty mysterious under the hood. Maybe it has to check whether w.near_miss_points has an append method every time when attempting to call it, and OTOH knows that locally-declared near_miss_points is a list? I don't know man, I didn't do it.

I'm undecided about whether to include this. Sometimes I like to make Python changes that mirror C++ changes, just to keep the two versions structured similarly. Other times I'll leave out changes with no clear benefit, to have a smaller impact on the commit history. Is there a clear benefit here? *Shrug.* At least we're down 14 lines of code?
The diff is at a768800. @jteresco, say the word and I can push it to this branch. Or leave it out.

yakra added 2 commits March 25, 2022 11:06
* construct NMP lists in place
* siteupdateST: restore Jim's original formula
The C++ part of 7b9a3b4c293d37887ca8aca43c228b121ced1e9e
The Python part of 7b9a3b4c293d37887ca8aca43c228b121ced1e9e
@yakrayakra added nmps Near-Miss Points speed labels Mar 25, 2022
@jteresco

Copy link
Copy Markdown
Contributor

I'm comfortable with whatever you decide on the Python code. But based on a quick read of your explanation, it would make sense to me to include the Python change also.

@yakra

Copy link
Copy Markdown
ContributorAuthor

Sounds good. Let's save those 14 lines of code, and that 0.93s when running datacheck.sh!
Pushed a768800 to this branch.

@jteresco
jteresco merged commit 1279af8 into TravelMapping:masterMar 25, 2022
@jteresco

Copy link
Copy Markdown
Contributor

This will be used for tonight's update.

@yakra
yakra deleted the nmp_split branch March 28, 2022 15:16
@yakrayakra mentioned this pull request Apr 12, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nmpsNear-Miss Pointsspeed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Single-threaded NMP detection: restore Jim's original formula WaypointQuadtree::near_miss_waypoints

2 participants

@yakra@jteresco