MPAS-Dev / MPAS

Repository for private MPAS development prior to the MPAS v6.0 release.
Other
4 stars 0 forks source link

Fix halo config pool side effects. #1502

Closed Larofeticus closed 6 years ago

Larofeticus commented 6 years ago

According to comment in https://github.com/MPAS-Dev/MPAS/pull/1501 config_pool entries created in halo exchanges are never used.

I grep'd around and could indeed find no matching get_config calls that accessed them.

I cataloged and reproduced all side effects of a halo exchange when I wrote the reuse routines. I did not think to second-guess the necessity of any of those side effects. The need arose because the reuse copying of pool config somehow disables the pool config's magical ability to free it's memory without naming it (the only time "% data" ever gets touched by a deallocate is in error recovery conditionals right after it was allocated).

The memory leak in the reuse code is gone. Also removes the vestigial config_pool stuff in the original halo exchanges. I've plugged it into a working ocean/develop and it works fine on a few iterations of mid-resolution.

mark-petersen commented 6 years ago

According to our previous testing, this fixes a known memory leak when using the halo reuse subroutine.

mgduda commented 6 years ago

@mark-petersen @Larofeticus I just noticed that this PR was created from Bill's develop branch. It might be preferable to create the PR from a branch.

mgduda commented 6 years ago

The changes look fine to me, and a quick test with the atmosphere core revealed no issues.

@Larofeticus In addition to moving this PR to its own branch (rather than creating the PR directly from your develop branch), I'd also request that you clean up the commit message so that lines are limited to about 80 columns. Depending on the terminal, git doesn't do well at wrapping long commit message lines, which end up truncated and therefore unreadable. For example, this is what I see when running git log:

commit 76d81d5355857ae1621554870acecc44cfce9be4
Author: Larofeticus <Larofeticus@gmail.com>
Date:   Tue Feb 20 09:25:57 2018 -0800

    remove unnessecary config pull side effects in halo exchange. remove duplication of those side effects in halo 

commit 9e729cd22576861484b71be98619ff87d2771d6a
Merge: 3e81e40 90f9417
Author: Xylar Asay-Davis <xylarstorm@gmail.com>
Date:   Tue Feb 6 00:29:48 2018 +0100

    Merge PR #1479 from xylar/framework/update_compass_scripts to develop

    Make COMPASS scripts PEP8 compliant

    This merge modifies COMPASS scripts to be PEP8 compliant and to produce PEP8 compliant output (at least to the 

    The four main scripts that drive the COMPASS testing infrastructure were written with tabs, long lines and many

    The following mode line (for vim) has been added to the bottom of each file:

    # vim: foldmethod=marker ai ts=4 sts=4 et sw=4 ft=python

    All empty except: clauses have been replaced with explicit exception names. Empty except clauses are considered

    Copying of the environment in subprocess calls has been removed, as this is the default behavior.

    Auto-generated scripts have been made PEP8 compliant to the extent possible. (The main PEP8 violations are that

    To accommodate the PEP8 line-length restriction, a new function print_case was added to list_testcases.py. With

commit 90f94175b2273b606c4f0f1f1e0091eea5f5f919
Author: Xylar Asay-Davis <xylarstorm@gmail.com>
Date:   Mon Dec 18 11:02:46 2017 +0100

    Fix PEP8 in COMPASS output scripts

    Also removes unneeded copying of environment in subprocess calls
    (both in COMPASS scripts and autogenerated output scripts).
Larofeticus commented 6 years ago

See #1515

@mark-petersen @mgduda

mark-petersen commented 6 years ago

This PR was replaced by #1515.