Skip to content

use memory mapped files for the peer buffer #12

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

Merged
merged 4 commits into from
Aug 14, 2016

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Aug 13, 2016

to preserve state between restarts with some simple unit tests

enum { header_size = 16 };
mapped_vector(char const* file, size_t const size)
: m_map(file, header_size + size * sizeof(T))
, m_size(*static_cast<size_t*>(m_map.data()))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be reinterpret_cast since you're transmuting the data rather than downcasting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, I double checked the C++ spec and static_cast can indeed be used to convert void* to T*. Learn something new every day.

@arvidn
Copy link
Contributor Author

arvidn commented Aug 13, 2016

one questionable thing, I think, is that the header size is not technically guaranteed to be a multiple of alignof(T) in mapped_vector. That should actually be pretty easy to fix.

return;
}

this->threadid = threadid;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the thread id around a bit here, to have it be available in the constructor, in order to create unique names for the mapped files

@ssiloti
Copy link
Contributor

ssiloti commented Aug 13, 2016

lgtm, it would probably be a good idea to check the last modified time of the file and discard it if it is too old.

@arvidn
Copy link
Contributor Author

arvidn commented Aug 13, 2016

Yeah, I was thinking about that too. I got a bit lazy and thought that the user could always just delete the file :)
maybe in another patch

@arvidn
Copy link
Contributor Author

arvidn commented Aug 13, 2016

ok, fixed the alignment of T too

enum { header_size = 16 };
// the header must be large enough to make the first element still be
// correctly aligned
static constexpr size_t header_size = std::max(size_t(16), alignof(T));
Copy link
Contributor

Choose a reason for hiding this comment

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

std::max isn't constexpr in C++11, I don't think the qualifier is necessary in this case.

@ssiloti
Copy link
Contributor

ssiloti commented Aug 14, 2016

lgtm

@arvidn arvidn force-pushed the memory-mapped-file branch from 771333b to cb2d2cf Compare August 14, 2016 15:34
@arvidn
Copy link
Contributor Author

arvidn commented Aug 14, 2016

rebased on top of master again

@arvidn arvidn merged commit dded993 into bittorrent:master Aug 14, 2016
@arvidn arvidn deleted the memory-mapped-file branch August 14, 2016 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants