Skip to content
Snippets Groups Projects

Recurse less in sort

Closed Pedro Gonnet requested to merge recurse_less_in_sort into master

Implements limiting the sort recursion depth to avoid cells that will never have a hydro interaction within or below them.

This will probably not impact the sort task speed, but should have an effect on the memory used for sort buffers.

@pdraper, can you run a quick benchmark of this change to look at memory size and CPU usage? Thanks!

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
1324 1325 }
1325 1326
1326 1327 /**
1328 * @brief Recurse down a pair of cells and mark the cells that have hydro
1329 * interactions.
1330 */
1331 void engine_mark_hydro_cells_rec(struct engine *e, struct cell *ci,
  • Pedro Gonnet added 1 commit

    added 1 commit

    • f0639e03 - rename flag to 'requires_hydro/starts_sorts' to make it more general.

    Compare with previous version

  • I'm ran an EAGLE_25/.1 with a couple of ranks on this branch and the master and I think it is fair to say the differences are larger than the effect. The CPU times (non-optimized runs) are the same and we see only small changes in the numbers of allocations and size of allocations. For instance here are plots of the size as histograms for the zero (fake) and 1st steps:

    step0-sizes

    step1-sizes

    and the same plots as cumulative sums:

    step0-sizes-cum

    step1-sizes-cum

    I have others (mostly small steps, which seem identical), but I think you get the point.

  • Author Developer

    Thanks, Peter, for running the numbers!

    If I understand them correctly though, peak memory use, which is the limiting factor, is 30% smaller?

  • Only on that 1st step (2nd plots), the 0th step, which is usually our peak memory step, we're a little larger.

  • Author Developer

    OK, I really don't understand how this change could allocate more memory :/

  • Could the metis decomposition have been different and lead to a different number of particles on that node?

  • Yes, probably the initial partition in this case. These are plots for just one rank. I'll run up the totals for all ranks when I get a moment.

  • Should have done this first. When I combine all the allocations for hydro sorts from both ranks, the memory use is pretty much identical. Just to be sure, we do expect a change in the number and size of hydro.sort labelled memory?

  • Author Developer

    Yes, we should, unless we have hydro tasks all the way down the tree.

    Are you running with gravity? If not, then the tree structure may well just end where the hydro interactions end and then we really shouldn't see any difference.

  • I have gravity and stars:

    mpirun -np $SLURM_NTASKS ../../swift_mpi --pin --cosmology --hydro --self-gravity --stars -t 8 eagle_25.yml -n 32
  • Ran both tests once more for sanity's sake and they give the same result.

  • Yes, we should, unless we have hydro tasks all the way down the tree.

    Sounds like the same situation I was in when attempting the same fix. And I am still confused as to why this is the case.

  • Pedro Gonnet added 1 commit

    added 1 commit

    • 6ffc7e4d - Report unnecessary recursions, note that this now no longer avoids them, it just reports them.

    Compare with previous version

  • Author Developer

    @pdraper, I made a quick change to report cases in which the code is recursing when it shouldn't have to. Can you run it and see if this outputs anything? Thanks!

  • Ran that and it reports 29,011 occurrences for all the 30 steps I run.

    Putting this in perspective there are at least 30,000,000 hydro.sort allocations in same period.

  • Author Developer

    Thanks for running that!

    OK, so I guess the effect isn't as big as we hoped it would be. Thoughts on how to proceed?

  • So we agreed you would finish the function comments and accept anyway.

  • Pedro Gonnet added 2 commits

    added 2 commits

    • 28ee0628 - clean up code and make sure we're also including task_subtype_stars_density tasks.
    • 351660ef - formatting.

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading