-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Reduce memory usage for raster source handling. #5572
Changes from 14 commits
d8d9ac8
62c8b70
d316ff9
e4aaf07
432d49e
a9fce74
eef0722
f9ee74d
a587b14
f36707d
542c3ba
17f32f4
9c1c842
fd0f1b6
ee177ef
9da6cf8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,16 +4,23 @@ | |
#include "util/coordinate.hpp" | ||
#include "util/exception.hpp" | ||
|
||
#include <boost/algorithm/string.hpp> | ||
#include <boost/algorithm/string/trim.hpp> | ||
#include <boost/assert.hpp> | ||
#include <boost/filesystem.hpp> | ||
#include <boost/filesystem/fstream.hpp> | ||
#include <boost/foreach.hpp> | ||
#include <boost/spirit/include/qi.hpp> | ||
#include <boost/spirit/include/qi_int.hpp> | ||
|
||
#include <storage/io.hpp> | ||
|
||
#include <iostream> | ||
#include <iterator> | ||
#include <list> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems the header is not used. Is this a left-over from debugging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your check and comment. |
||
#include <string> | ||
#include <unordered_map> | ||
using namespace std; | ||
|
||
namespace osrm | ||
{ | ||
|
@@ -43,37 +50,31 @@ class RasterGrid | |
xdim = _xdim; | ||
ydim = _ydim; | ||
_data.reserve(ydim * xdim); | ||
BOOST_ASSERT(ydim * xdim <= _data.capacity()); | ||
|
||
// Construct FileReader | ||
storage::io::FileReader file_reader(filepath, storage::io::FileReader::HasNoFingerprint); | ||
|
||
std::string buffer; | ||
buffer.resize(file_reader.GetSize()); | ||
|
||
BOOST_ASSERT(buffer.size() > 1); | ||
|
||
file_reader.ReadInto(&buffer[0], buffer.size()); | ||
|
||
boost::algorithm::trim(buffer); | ||
|
||
auto itr = buffer.begin(); | ||
auto end = buffer.end(); | ||
|
||
bool r = false; | ||
try | ||
{ | ||
r = boost::spirit::qi::parse( | ||
itr, end, +boost::spirit::qi::int_ % +boost::spirit::qi::space, _data); | ||
} | ||
catch (std::exception const &ex) | ||
{ | ||
throw util::exception("Failed to read from raster source " + filepath.string() + ": " + | ||
ex.what() + SOURCE_REF); | ||
} | ||
buffer.resize(xdim * 11); // INT32_MAX = 2147483647 = 10 chars + 1 white space = 11 | ||
BOOST_ASSERT(xdim * 11 <= buffer.size()); | ||
|
||
if (!r || itr != end) | ||
for (unsigned int y = 0; y < ydim; y++) | ||
{ | ||
throw util::exception("Failed to parse raster source: " + filepath.string() + | ||
SOURCE_REF); | ||
// read one line from file. | ||
file_reader.ReadLine(&buffer[0], xdim * 11); | ||
boost::algorithm::trim(buffer); | ||
|
||
std::vector<std::string> result; | ||
boost::split( | ||
result, buffer, boost::is_any_of(" \r\n\0"), boost::algorithm::token_compress_on); | ||
unsigned int x = 0; | ||
for (const auto &s : result) | ||
{ | ||
if (x < xdim) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, I afraid the difference of width (= the number of pixels) between actual raster file and setting in lua. To avoid this situation, I put the Which do you prefer? Best Regards, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HI! Thank you for the explanation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, |
||
_data[(y * xdim) + x] = atoi(s.c_str()); | ||
++x; | ||
} | ||
BOOST_ASSERT(x == xdim); | ||
} | ||
} | ||
|
||
|
@@ -143,8 +144,32 @@ class RasterContainer | |
RasterDatum GetRasterInterpolateFromSource(unsigned int source_id, double lon, double lat); | ||
|
||
private: | ||
}; | ||
|
||
// << singletone >> RasterCache | ||
class RasterCache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you design this class as a singleton? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, This is most important point of my pull request.
Best Regards, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi! It seems that RasterContainer binds directly to the lua functions https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/scripting_environment_lua.cpp#L188 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, This is usage of I think we cannot avoid to create more than one RasterContainer instance, since Best Regards, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, The instance of RasterContainer is created at here. So, we cannot control the creation of RasterContainer instance in lua files (such as rasterbot.lua). Best Regards, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, I briefly checked scripting_environment_lua.{hpp|cpp}. If we want to avoid multiple load of same raster file, there are two strategies. Best Regards, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, thank you for this research. Now I am not against A) strategy. Can you add a briefly comment before this class to avoid confusion of future readers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, |
||
{ | ||
public: | ||
// class method to get the instance | ||
static RasterCache &getInstance() | ||
{ | ||
if (NULL == g_instance) | ||
{ | ||
g_instance = new RasterCache(); | ||
gardster marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return *g_instance; | ||
} | ||
// get reference of cache | ||
std::vector<RasterSource> &getLoadedSources() { return LoadedSources; } | ||
std::unordered_map<std::string, int> &getLoadedSourcePaths() { return LoadedSourcePaths; } | ||
private: | ||
// constructor | ||
RasterCache() = default; | ||
// member | ||
std::vector<RasterSource> LoadedSources; | ||
std::unordered_map<std::string, int> LoadedSourcePaths; | ||
// the instance | ||
static RasterCache *g_instance; | ||
}; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
#include "util/log.hpp" | ||
#include "util/version.hpp" | ||
|
||
#include <boost/filesystem.hpp> | ||
#include <boost/filesystem/fstream.hpp> | ||
#include <boost/iostreams/device/array.hpp> | ||
#include <boost/iostreams/seek.hpp> | ||
|
@@ -60,29 +61,27 @@ class FileReader | |
|
||
std::size_t GetSize() | ||
{ | ||
const boost::filesystem::ifstream::pos_type position = input_stream.tellg(); | ||
input_stream.seekg(0, std::ios::end); | ||
const boost::filesystem::ifstream::pos_type file_size = input_stream.tellg(); | ||
|
||
if (file_size == boost::filesystem::ifstream::pos_type(-1)) | ||
const boost::filesystem::path path(filepath); | ||
try | ||
{ | ||
throw util::RuntimeError("Unable to determine file size for " + | ||
std::string(filepath.string()), | ||
ErrorCode::FileIOError, | ||
SOURCE_REF, | ||
std::strerror(errno)); | ||
return std::size_t(boost::filesystem::file_size(path)) - | ||
((fingerprint == FingerprintFlag::VerifyFingerprint) ? sizeof(util::FingerPrint) | ||
: 0); | ||
} | ||
|
||
// restore the current position | ||
input_stream.seekg(position, std::ios::beg); | ||
|
||
if (fingerprint == FingerprintFlag::VerifyFingerprint) | ||
catch (boost::filesystem::filesystem_error &ex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, thanks for your comment. |
||
{ | ||
return std::size_t(file_size) - sizeof(util::FingerPrint); | ||
std::cout << ex.what() << std::endl; | ||
throw; | ||
} | ||
else | ||
} | ||
|
||
/* Read one line */ | ||
template <typename T> void ReadLine(T *dest, const std::size_t count) | ||
{ | ||
if (0 < count) | ||
{ | ||
return file_size; | ||
memset(dest, 0, count * sizeof(T)); | ||
input_stream.getline(reinterpret_cast<char *>(dest), count * sizeof(T)); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems the header is not used. Is this a left-over from debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your check and comment.
I will remove it after confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Mr. DennisOSRM,
Thanks for your strong support.
May I ask you to proceed reviewing?
I have already modified the code and committed.
Best Regards,
Tomonobu Saito
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good from my side 👍