treee111 / wahooMapsCreator

Create maps for Wahoo device based on latest OSM maps
247 stars 25 forks source link

Phyghtmap is not forking any jobs when generating contour lines, due to max-nodes-per-tile set to 0 #209

Open alfh opened 1 year ago

alfh commented 1 year ago

Expected Behavior

Changing number of jobs argument for phyghtmap, should lead to difference in how many subprocesses are created.

Current Behavior

As far as I can see from the code, and when looking at the log output, the forking is not influenced by the arguments. So I think the jobs argument should either be removed, or set to 1, and also comment that it does not help to try to change it.

I do not understand the phyghtmap well enough, to judge if the max-nodes-per-tile=0 is strictly needed.

Steps to Reproduce the Issue

When the phyghtmap is started, the python code is using this code in osm_maps_functions.py

                cmd.extend(['-o', f'{out_file_elevation}', '-s 10', '-c 100,50', elevation_source,
                            '--jobs=8', '--viewfinder-mask=1', '--start-node-id=20000000000',
                            '--max-nodes-per-tile=0', '--start-way-id=2000000000', '--write-timestamp',
                            '--no-zero-contour', '--hgtdir=' + hgt_path])

But the options "jobs" is really disregarded by and "max-nodes-per-tile", this is the relevant code from phyghtmap

        if opts.maxNodesPerTile == 0:
                singleOutput = True
        else:
                singleOutput = False

and

        def process(self):
                if self.singleOutput:
                        self.processSingleOutput()
                else:
                        self.processMultiOutput()

Context

These comments apply for Linux.

For Windows, phyghtmap does not support any threading at all, since it check for the "fork" support.

        if hasattr(os, "fork") and opts.nJobs != 1:

Since for example the 133/75 tile takes 110 seconds to generate the contour lines, and the CPU usage indicated that only one core is being used, there are great performance improvement potential when using phyghtmap.

But when I tried running phyghtmap manually from command line, and without the "max-nodes-per-tile=0" and with jobs set to 2 or higher, this also did not seem to have any real effects. The log output seemed to indicate that it was processing only one file at the time.

So it seems like we might have to look at setting things up for paralell executing from wahoomc, by using asyncio, and invoking phyghtmap for different tiles in paralell, but that should be covered in another issue.

Log Output / Stack Trace

treee111 commented 1 year ago

Hi @alfh, thanks for the great analysis!

Did I get it right that you are referring to two things:

  1. Because we give the argument --max-nodes-per-tile=0 to phyghtmap, the argument --jobs=8 is not taken into account.
    • I found this also the documentation:
      phyghtmap -a 6:44:9:47 -j 2 --max-nodes-per-tile=0 --max-nodes-per-way=0 --gzip=9
      This generates contour lines for the high alps and writes them to a single output file.
      Note that it is possible to use multiple processes in parallel. --max-nodes-per-way=0 means that the ways will be as long as possible. --gzip=9 will produce gzipped output with a compression level of 9. 
    • I don't know why we have =0 here, does it mean it has more nodes in the output files as e.g. =100000 would produce and hence a higher detail level? Could be checked by comparing the output size of different calls.
    • @masc4ii do you know why we have --max-nodes-per-tile=0 and that in this combination the jobs argument does hase an affect?
  2. When --max-nodes-per-tile=0 is not given, the jobs argument does also not have an effect on the used jobs/processors or similar
    • we should think of a solution to parallelize phyghtmap (and other possible processings) from wahoomc like drafted in #197.
    • Thinking about what might be possible to run in parallel and what not took me to what read and/or write operations are done by each processing. Just a draft, not tested
    • If the parallel calls would write/download the same files parallelization might not be possible
    • If the parallel calls would read the same files that might be possible
masc4ii commented 1 year ago

If I remember right, the output lines were looked very limited (lines not round anymore, like drawn with just a few connected points) when not using --max-nodes-per-tile=0 --max-nodes-per-way=0. I tried that for a tile in the alps and it looked very bad.

treee111 commented 1 year ago

thanks for the reply! That makes sense, so I would leave it as-is

alfh commented 1 year ago

I had only looked at this for performance perspective, I did not check any functional effects of --max-nodes-per-tile=0.

So I agree that --max-nodes-per-tile=0 should be kept, but then the --jobs should either be set to 1 or be removed, to not pretend that it is multi threaded.

I did not even see any noticable improvements even when using --jobs=12 and having 16 vCPU, it seems like only some small portion of the phyghtmap can take advantage for multi CPU.

So in general, I think the performance for the wahoomapscreator can be increased with the minimum code effort by processing most of the steps for one tile in parallel, but I'll give some input on that in the exisintg specific issue about scalability.