Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dsl: No more dynamic classes for AbstractFunctions #2190

Merged
merged 12 commits into from
Aug 23, 2023

Conversation

FabioLuporini
Copy link
Contributor

This PR revamps the way AbstractFunctions are created and managed by the runtime.

We stop using dynamic classes for each new AbstractFunction the user creates. This has several advantages:

  • simpler code
  • no need for our own caching for AbstractFunction !
    • maybe we can drop our own internal cache entirely at some point too, because why not? as long as we suitably override _hashable_content, I don't see why this shouldn't be possible. This would be a massive simplification. Anyway, this is for another day.
  • fixes a long-standing tiny leak due to the fact that dynamic classes once created cannot be garbage collected. This is a known python thing. The order of magnitude of the leak per AbstractFunction is O(bytes), which could turn into a problem if the creation occurs within deeply nested loops (e.g. a-la FWI)

Also:

  • fix another memory leak buried inside codepy or the way we use codepy. Not sure, but most likely the former.

@ofmla : you bumped into issue #845 at some point. Could you confirm that this PR does indeed fix the issue? It does according to my tests, but having external people reproduce it would make me a bit more comfortable. Thanks anyway!

Fixes #845 #1859

@FabioLuporini FabioLuporini requested a review from mloubout August 21, 2023 08:12
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #2190 (25c856a) into master (67e5779) will decrease coverage by 0.04%.
The diff coverage is 91.54%.

@@            Coverage Diff             @@
##           master    #2190      +/-   ##
==========================================
- Coverage   87.10%   87.06%   -0.04%     
==========================================
  Files         226      228       +2     
  Lines       40197    40299     +102     
  Branches     7335     7357      +22     
==========================================
+ Hits        35014    35088      +74     
- Misses       4603     4625      +22     
- Partials      580      586       +6     
Files Changed Coverage Δ
tests/test_gpu_common.py 1.44% <0.00%> (ø)
devito/finite_differences/derivative.py 90.37% <76.00%> (-6.14%) ⬇️
devito/mpatches/rationaltools.py 78.57% <78.57%> (ø)
devito/arch/compiler.py 40.00% <80.00%> (+0.62%) ⬆️
devito/symbolics/printer.py 85.52% <83.33%> (+0.29%) ⬆️
devito/types/basic.py 93.24% <97.50%> (-0.11%) ⬇️
devito/__init__.py 94.59% <100.00%> (+0.07%) ⬆️
devito/builtins/arithmetic.py 97.64% <100.00%> (ø)
devito/finite_differences/differentiable.py 94.19% <100.00%> (ø)
devito/mpatches/__init__.py 100.00% <100.00%> (ø)
... and 11 more

... and 4 files with indirect coverage changes

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mloubout mloubout force-pushed the no-more-dyn-classes-final branch 2 times, most recently from fd5b167 to 174dc8b Compare August 22, 2023 16:20
@mloubout mloubout changed the title compiler: No more dynamic classes for AbstractFunctions dsl: No more dynamic classes for AbstractFunctions Aug 22, 2023
@mloubout mloubout added the API api (symbolics, types, ...) label Aug 22, 2023
@mloubout mloubout force-pushed the no-more-dyn-classes-final branch from 174dc8b to 28db13e Compare August 22, 2023 19:17
devito/mpatches/.rationaltools.py.swp Outdated Show resolved Hide resolved

def __init_finalize__(self, *args, **kwargs):
super(AbstractSparseFunction, self).__init_finalize__(*args, **kwargs)
self._npoint = kwargs['npoint']
self._npoint = kwargs.get('npoint', kwargs.get('npoint_global'))
Copy link
Contributor

Choose a reason for hiding this comment

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

CAn we stick to just one, not sure why we need both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's because of issue #1498 . Same story as shape vs shape_global. Here npoint is the decomposed on. But to reconstruct, we need npoint_global.

I think a potentially good, potentially game-changing question to ask ourselves is the following: why, when we reconstruct, do we even care about carrying around the following attribute:

  • shape
  • data
  • distributor
  • ...

In other words, I'm now thinking that only the "main" user-constructed should have this info. All other reconstructions get it via .function if they really need to; but most importantly, since reconstructions are performed by the compiler, and since the compiler doesn't clearly care about shape or data, why the hell do we even need all this maddness?

IOW, TLDR: there might be a subset of attributes that are useless at reconstruction time.

That said, I'm diverging here, perhaps food for thought for another day

@mloubout mloubout force-pushed the no-more-dyn-classes-final branch from 28db13e to 25c856a Compare August 23, 2023 12:41
@mloubout mloubout merged commit 7763d03 into master Aug 23, 2023
@mloubout mloubout deleted the no-more-dyn-classes-final branch August 23, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API api (symbolics, types, ...) bug-py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory consumption gradually grows when Operator called in loop
2 participants