Skip to content
This repository has been archived by the owner on Oct 23, 2020. It is now read-only.

Fix halo config pool side effects. #1502

Closed
wants to merge 1 commit into from

Conversation

Larofeticus
Copy link
Contributor

@Larofeticus Larofeticus commented Feb 20, 2018

According to comment in #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.

…duplication of those side effects in halo reuse which also leaks memory
@Larofeticus Larofeticus changed the title Fix halo config pull side effects. Fix halo config pool side effects. Feb 20, 2018
@mark-petersen mark-petersen self-requested a review February 26, 2018 18:35
Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test compiled with both gnu and intel/debug in develop and ocean/develop. Passes all ocean tests on ocean/develop.

@mark-petersen
Copy link
Contributor

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

@mgduda mgduda self-requested a review March 12, 2018 17:13
@mgduda
Copy link
Contributor

mgduda commented Mar 12, 2018

@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
Copy link
Contributor

mgduda commented Mar 12, 2018

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).

@mgduda mgduda closed this Mar 12, 2018
@mgduda mgduda reopened this Mar 12, 2018
Copy link
Contributor

@mgduda mgduda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR should be moved to its own branch, and the commit message should be cleaned up to:

  1. Use lines <= 80 characters
  2. Provide a description that would be more directly useful to those not involved in original discussion of these changes
  3. Correct spelling ("nessecary"), etc.

@Larofeticus
Copy link
Contributor Author

See #1515

@mark-petersen
@mgduda

@mark-petersen
Copy link
Contributor

This PR was replaced by #1515.

mark-petersen added a commit that referenced this pull request Mar 14, 2018
According to comment in #1501 config_pool entries created in halo
exchanges are no longer used and can be removed. Config_pool entries
when ported to halo data structure reuse also cause a memory leak.

According to comment in #1502 the code builds and passes testing.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants