Skip to content

Commit

Permalink
cleanups (TODO: See if memory locking helps)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pcmoritz
pcmoritz committed Aug 18, 2017
1 parent 4702703 commit c52f211
Show file tree
Hide file tree
Showing 6 changed files with 13 additions and 52 deletions.
2 changes: 1 addition & 1 deletion cpp/src/plasma/client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ uint8_t* PlasmaClient::lookup_or_mmap(int fd, int store_fd_val, int64_t map_size
if (result == MAP_FAILED) {
ARROW_LOG(FATAL) << "mmap failed";
}
close(fd); // PERF: closing this fd has an effect on performance.
close(fd); // Closing this fd has an effect on performance.

ClientMmapTableEntry& entry = mmap_table_[store_fd_val];
entry.pointer = result;
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/plasma/common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ extern int ObjectStatusLocal;
extern int ObjectStatusRemote;

/// Globally accessible reference to plasma store configuration.
/// TODO: may be avoided with some refactoring of existing code.
/// TODO(pcm): This can be avoided with some refactoring of existing code
/// by making it possible to pass a context object through dlmalloc.
struct PlasmaStoreInfo;
extern const PlasmaStoreInfo* plasma_config;
} // namespace plasma
Expand Down
30 changes: 3 additions & 27 deletions cpp/src/plasma/malloc.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@
#include <unistd.h>

#include <unordered_map>
#include <iostream>
#include <cerrno>
#include <cstring>

#include "plasma/common.h"
#include "plasma/plasma.h"
Expand All @@ -44,7 +42,7 @@ int fake_munmap(void*, int64_t);
#define USE_DL_PREFIX
#define HAVE_MORECORE 0
#define DEFAULT_MMAP_THRESHOLD MAX_SIZE_T
#define DEFAULT_GRANULARITY ((size_t) 1024U * 1024U * 1024U) //1GB
#define DEFAULT_GRANULARITY ((size_t) 1024U * 1024U * 1024U) // 1GB

#include "thirdparty/dlmalloc.c" // NOLINT

Expand Down Expand Up @@ -93,17 +91,7 @@ int create_buffer(int64_t size) {
fd = -1;
}
#else
#ifdef __linux__
if (plasma::plasma_config->hugetlb_enabled) {
file_template += "/hugepagefile";
} else {
file_template += "/plasmaXXXXXX"; // template
}
// constexpr char file_template[] = "/dev/shm/plasmaXXXXXX";
#else
// constexpr char file_template[] = "/tmp/plasmaXXXXXX";
file_template += "/plasmaXXXXXX";
#endif
std::vector<char> file_name(file_template.begin(), file_template.end());
file_name.push_back('\0');
if (plasma::plasma_config->hugetlb_enabled) {
Expand All @@ -113,27 +101,23 @@ int create_buffer(int64_t size) {
fd = mkstemp(&file_name[0]);
}
if (fd < 0) {
perror("create_buffer: open failed");
ARROW_LOG(FATAL) << "create_buffer failed to open file " << &file_name[0];
return -1;
}

FILE* file = fdopen(fd, "a+");
if (!file) {
perror("create_buffer: fdopen failed");
close(fd);
ARROW_LOG(FATAL) << "create_buffer: fdopen failed for " << &file_name[0];
return -1;
}
if (unlink(&file_name[0]) != 0) {
perror("create_buffer: failed to unlink file");
ARROW_LOG(FATAL) << "unlink error";
ARROW_LOG(FATAL) << "failed to unlink file " << &file_name[0];
return -1;
}
if (!plasma::plasma_config->hugetlb_enabled) {
if (ftruncate(fd, (off_t)size) != 0) {
perror("create_buffer: failed to truncate file");
ARROW_LOG(FATAL) << "ftruncate error";
ARROW_LOG(FATAL) << "failed to ftruncate file " << &file_name[0];
return -1;
}
}
Expand All @@ -154,14 +138,6 @@ void* fake_mmap(size_t size) {
ARROW_LOG(ERROR) << "mmap failed with error : " << std::strerror(errno);
return pointer;
}
// Attempt to mlock the mmaped region of memory (best effort).
int rv = mlock(pointer, size);

This comment has been minimized.

Sign in to view

Copy link
@atumanov

atumanov Aug 18, 2017

just to reiterate, I think it's important to keep the mlock/memset sequence as best effort performance optimization in this PR. They should be allowed to fail. If they succeed, they will have the desired effect of pre-polulating page frames. Yes, I'm aware of MAP_POPULATE. It doesn't have the same (positive) effect on performance for me. One variation here is to attempt a memset only on mlock failure.
if (rv != 0) {
ARROW_LOG(WARNING) << "mlock failed with error : " << std::strerror(errno);
}
ARROW_LOG(INFO) << "mlocking pointer " << pointer << " size " << size
<< " success " << rv;
memset(pointer, 0xff, size);

/* Increase dlmalloc's allocation granularity directly. */
mparams.granularity *= GRANULARITY_MULTIPLIER;
Expand Down
1 change: 0 additions & 1 deletion cpp/src/plasma/plasma.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ namespace plasma {

/// Allocation granularity used in plasma for object allocation.
#define BLOCK_SIZE 64
#define DEFAULT_HUGETLBFS_MOUNTDIR "/mnt/hugepages"

struct Client;

Expand Down
24 changes: 5 additions & 19 deletions cpp/src/plasma/store.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,7 @@ PlasmaStore::~PlasmaStore() {
}
}

// Get a const reference to the internal PlasmaStoreInfo object.
const PlasmaStoreInfo& PlasmaStore::getPlasmaStoreInfoRef() {
return store_info_;
}

// Get a const pointer to the internal PlasmaStoreInfo object.
const PlasmaStoreInfo* PlasmaStore::getPlasmaStoreInfoPtr() {
const PlasmaStoreInfo* PlasmaStore::get_plasma_store_info() {

This comment has been minimized.

Sign in to view

Copy link
@atumanov

atumanov Aug 18, 2017 •
edited
Loading

I felt like getPlasmaStoreInfoPtr is appropriate, to differentiate between getting a reference and a pointer. While we don't need both for this PR, we may want both in the future. Why not have both for completeness of the PlasmaStore API?
return &store_info_;
}

Expand Down Expand Up @@ -652,12 +646,10 @@ class PlasmaStoreRunner {
loop_.reset(new EventLoop);
store_.reset(new PlasmaStore(loop_.get(), system_memory, directory,
hugetlb_enabled));
plasma_config = store_->getPlasmaStoreInfoPtr();
plasma_config = store_->get_plasma_store_info();
int socket = bind_ipc_sock(socket_name, true);
// TODO(pcm): Check return value.
ARROW_CHECK(socket >= 0);
// Pre-warm the shared memory store.
ARROW_CHECK(dlmalloc(1024) != NULL);

This comment has been minimized.

Sign in to view

Copy link
@atumanov

atumanov Aug 18, 2017

If you remove this dlmalloc, the initial file will not be allocated on plasma store startup. I think it's best to do as much setup for highest performance as possible on startup. As we discussed, part of a separate PR could be mmapping as much of the configured memory capacity as possible on plasma store startup.

loop_->AddFileEvent(socket, kEventLoopRead, [this, socket](int events) {
this->store_->connect_client(socket);
Expand Down Expand Up @@ -740,20 +732,14 @@ int main(int argc, char* argv[]) {
ARROW_LOG(FATAL) << "please specify the amount of system memory with -m switch";
}
if (memfile_directory.empty()) {
ARROW_LOG(WARNING) << "Memory-backed file directory not specified.";
// Backward compatibility: deduce directory name automatically.
#ifdef __linux__
if (hugetlb_enabled) {
memfile_directory = DEFAULT_HUGETLBFS_MOUNTDIR;
} else {
memfile_directory = "/dev/shm";
}
memfile_directory = "/dev/shm";
#else
memfile_directory = "/tmp";

This comment has been minimized.

Sign in to view

Copy link
@atumanov

atumanov Aug 18, 2017

I don't think this change makes sense. If -h is specified , but not -d, on linux you will get /dev/shm, which is confusing to the user who's just explicitly requested hugetlbfs on the command line.
#endif
}
ARROW_LOG(INFO) << "Starting object store in directory " << memfile_directory
<< " and HUGETLBFS " << (hugetlb_enabled?"enabled":"disabled");
ARROW_LOG(INFO) << "Starting object store with directory " << memfile_directory
<< " and huge page support " << (hugetlb_enabled ? "enabled" : "disabled");
#ifdef __linux__
// On Linux, check that the amount of memory available in /dev/shm is large
// enough to accommodate the request. If it isn't, then fail.
Expand Down
5 changes: 2 additions & 3 deletions cpp/src/plasma/store.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ class PlasmaStore {

~PlasmaStore();

/// Get a const reference to the internal PlasmaStoreInfo object.
const PlasmaStoreInfo& getPlasmaStoreInfoRef();
const PlasmaStoreInfo* getPlasmaStoreInfoPtr();
/// Get a const pointer to the internal PlasmaStoreInfo object.
const PlasmaStoreInfo* get_plasma_store_info();

/// Create a new object. The client must do a call to release_object to tell
/// the store when it is done with the object.
Expand Down

0 comments on commit c52f211

Please sign in to comment.