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

mmap()'d Ring Space #135

Open
wants to merge 57 commits into
base: master
Choose a base branch
from

Conversation

jaycedowell
Copy link
Collaborator

@jaycedowell jaycedowell commented Feb 3, 2020

This PR adds a new mmap()'d space, named "mapped", for Bifrost rings. This will allow for larger disk-based rings for buffering data.

@jaycedowell
Copy link
Collaborator Author

I'm generally happy with how it works but the specification of the base directory in user.mk seems a little fiddly. I'm also not sure how to allow multiple base directories to be specified.

@coveralls
Copy link

coveralls commented Feb 3, 2020

Coverage Status

Coverage decreased (-0.03%) to 61.338% when pulling 4ff90d0 on jaycedowell:disk-backed-ring into 1681fde on ledatelescope:master.

@codecov-io
Copy link

codecov-io commented Feb 3, 2020

Codecov Report

Merging #135 into master will decrease coverage by 0.21%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   58.78%   58.56%   -0.22%     
==========================================
  Files          63       64       +1     
  Lines        5095     5112      +17     
==========================================
- Hits         2995     2994       -1     
- Misses       2100     2118      +18
Impacted Files Coverage Δ
python/bifrost/pipeline.py 84.38% <ø> (ø) ⬆️
python/bifrost/libbifrost.py 55.07% <ø> (ø) ⬆️
python/bifrost/ndarray.py 77.23% <100%> (ø) ⬆️
python/bifrost/Space.py 61.11% <100%> (ø) ⬆️
python/bifrost/memory.py 69.64% <50%> (-4.44%) ⬇️
python/bifrost/romein.py 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2af28de...1d755d8. Read the comment docs.

Copy link
Collaborator

@telegraphic telegraphic left a comment

Choose a reason for hiding this comment

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

Seems like useful functionality and at a glance all looks in order to me. Again would defer on changes to memory.cpp to others for review.

Random note: flock can act strangely over NFS mounts. But I think using an NFS mount as a disk buffer is a pretty unlikely choice and you'd kinda expect it not to work anyways.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #135 into master will decrease coverage by 2.90%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   61.62%   58.71%   -2.91%     
==========================================
  Files          64       64              
  Lines        5300     5302       +2     
==========================================
- Hits         3266     3113     -153     
- Misses       2034     2189     +155     
Impacted Files Coverage Δ
python/bifrost/libbifrost.py 52.70% <ø> (-25.68%) ⬇️
python/bifrost/pipeline.py 84.66% <ø> (ø)
python/bifrost/memory.py 70.17% <50.00%> (-6.19%) ⬇️
python/bifrost/Space.py 61.11% <100.00%> (ø)
python/bifrost/ndarray.py 76.92% <100.00%> (-5.99%) ⬇️
python/bifrost/fir.py 0.00% <0.00%> (-100.00%) ⬇️
python/bifrost/romein.py 0.00% <0.00%> (-100.00%) ⬇️
python/bifrost/blocks/scrunch.py 37.03% <0.00%> (-59.26%) ⬇️
python/bifrost/map.py 20.83% <0.00%> (-56.25%) ⬇️
python/bifrost/linalg.py 36.84% <0.00%> (-52.64%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a53b35b...3abcc4a. Read the comment docs.

@jaycedowell
Copy link
Collaborator Author

I think the environment variable approach is viable and takes care of my last concern.

@jaycedowell
Copy link
Collaborator Author

Well, at least mapped compiles on MacOS now. That's something.

@jaycedowell
Copy link
Collaborator Author

Keep the logging directory and the mapped ring directory separate on mac seems to have helped with a lot of the testing failures but not there is a segfault when the test suite exits.

jaycedowell added a commit to jaycedowell/bifrost that referenced this pull request Mar 25, 2022
@league league mentioned this pull request Mar 25, 2022
jaycedowell and others added 6 commits March 29, 2022 09:11
(Noticed this ×3 in CI.)

```
memory.cpp: In destructor 'MappedMgr::~MappedMgr()':
memory.cpp:130:17: warning: catching polymorphic
   type 'class std::exception' by value [8;;
   https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wcatch-value=
   -Wcatch-value=8;;]
  130 |   } catch( std::exception ) {}
      |                 ^~~~~~~~~
```
@jaycedowell
Copy link
Collaborator Author

Looks like there are some problems related to the recent changes to filesystem.[ch]pp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants