-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 all 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,21 @@ | |
#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 <iterator> | ||
#include <string> | ||
#include <unordered_map> | ||
using namespace std; | ||
|
||
namespace osrm | ||
{ | ||
|
@@ -43,37 +48,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) | ||
_data[(y * xdim) + x] = atoi(s.c_str()); | ||
++x; | ||
} | ||
BOOST_ASSERT(x == xdim); | ||
} | ||
} | ||
|
||
|
@@ -143,8 +142,36 @@ class RasterContainer | |
RasterDatum GetRasterInterpolateFromSource(unsigned int source_id, double lon, double lat); | ||
|
||
private: | ||
}; | ||
|
||
// << singletone >> RasterCache | ||
// The instance of RasterContainer is created for every threads osrm-extract uses. | ||
// To avoid multiple load of same file on each RasterContainer, | ||
// The LoadedSources and LoadedSourcePaths are separated to RasterCache class | ||
// and handled as the singletone pattern to avoid duplicate creation. | ||
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; | ||
}; | ||
} | ||
} | ||
|
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!
Do we need this check if we will cover it with
BOOST_ASSERT(x == xdim);
? Maybe this check can also be assertion?I'm not confident with the file format and maybe it is normal to have more values in row than we expect in header: then assertion is not valid.
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,
Thanks for your comment.
I afraid the difference of width (= the number of pixels) between actual raster file and setting in lua.
Especially, if (width of actual file) < (setting in lua) then we will face buffer overflow reading.
As you know, the result is not defined (beyond of control).
To avoid this situation, I put the
if (x < xdim)
checking, just in case.If you prefer to remove this checking, I can remove.
Which do you prefer?
a) leave
if (x < xdim)
just in case.b) remove. we believe user's setting in lua :)
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.
HI! Thank you for the explanation.
Now I understand that we got xdim setting from the lua profile and it can be a source of mistakes.
I think we should keep this check.
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,
Thanks for your comment.
OK, I keep this modification.