Correct some uses of wrong variable types preventing QLever from running on ARM #116
Conversation
src/index/IndexMetaData.h
Outdated
| @@ -153,7 +154,7 @@ using IndexMetaDataMmapView = IndexMetaData< | |||
| MetaDataWrapperDense<ad_utility::MmapVectorView<FullRelationMetaData>>>; | |||
|
|
|||
| // constants for Magic Numbers to separate different types of MetaData; | |||
| const size_t MAGIC_NUMBER_MMAP_META_DATA = static_cast<size_t>(-1); | |||
| const size_t MAGIC_NUMBER_SPARSE_META_DATA = static_cast<size_t>(-2); | |||
| const uint64_t MAGIC_NUMBER_MMAP_META_DATA = std::numeric_limits<uint64_t>::max(); | |||
There was a problem hiding this comment.
This still has wrong formatting
src/util/MmapVector.h
Outdated
| @@ -252,8 +252,8 @@ class MmapVector { | |||
| std::string _filename = ""; | |||
| AccessPattern _pattern = AccessPattern::None; | |||
| static constexpr float ResizeFactor = 1.5; | |||
| static constexpr int MagicNumber = 7601577; | |||
| static constexpr int Version = 0; | |||
| static constexpr uint64_t MagicNumber = 7601577; | |||
There was a problem hiding this comment.
This changes the index format, will change to use uint32_t here which should be compatible
| @@ -77,14 +80,14 @@ void MmapVector<T>::readMetaDataFromEnd() { | |||
| template <class T> | |||
| void MmapVector<T>::mapForReading() { | |||
| // open to get valid file descriptor | |||
| FILE* fp = fopen(_filename.c_str(), "r"); | |||
| int orig_fd = ::open(_filename.c_str(), O_RDONLY); | |||
There was a problem hiding this comment.
This is just to make things more similar to the write case
* These break building QLever on ARM where size_t is incompatible with Id (uint64_t) * Most are uses of size_t where we really want Id * Also use explicitly sized types for versions and magic numbers * In MmapVector we were also using -someSizeT as an off_t which somehow doesn't work on ARM. I guess the compiler tries to do it as an unary minus before converting to off_t
src/util/MmapVectorImpl.h
Outdated
| @@ -43,6 +43,8 @@ void MmapVector<T>::writeMetaDataToEnd() { | |||
| ofs.write((char*)&_bytesize, sizeof(_bytesize)); | |||
| ofs.write((char*)&MagicNumber, sizeof(MagicNumber)); | |||
| ofs.write((char*)&Version, sizeof(Version)); | |||
| ofs.flush(); | |||
There was a problem hiding this comment.
I see this pattern over and over again in the code:
Usage of explicit open() and close() for ad_utility::File and std::fstream.
But actually I think this not necessary when the file class is only used locally and the flush and close are done automagically by the destructor. So wouldn't it be more C++ to let RAII do its job here?
There was a problem hiding this comment.
Makes sense, I played around with this searching for a bug that broke the MmapVectorTest. Ultimately it was something else. So yeah makes sense to let that happen implicitly
| @@ -77,14 +80,14 @@ void MmapVector<T>::readMetaDataFromEnd() { | |||
| template <class T> | |||
| void MmapVector<T>::mapForReading() { | |||
| // open to get valid file descriptor | |||
| FILE* fp = fopen(_filename.c_str(), "r"); | |||
| int orig_fd = ::open(_filename.c_str(), O_RDONLY); | |||
| @@ -588,22 +588,20 @@ string decodeUrl(const string& url) { | |||
| } else if (h1 >= 'a' && h1 <= 'f') { | |||
| h1 = h1 - 'a' + 10; | |||
| } else { | |||
| h1 = -1; | |||
| decoded += '%'; | |||
There was a problem hiding this comment.
Is this indeed valid URL encoding? As I see it you interpret % as % when the next character cannot be interpreted as HEX. Is there a reason for this?
There was a problem hiding this comment.
I see, you just refactored the original behaviour. I'm still not sure whether this is within the specification.
There was a problem hiding this comment.
Neither am I. It's a bit hackish too.
To improve the portability and type correctness of the QLever code base I'm trying to get it running on my Raspberry Pi (ARM) as well as an Odroid C1 (ARM64). I find that most issues this uncovers look like they are worth doing right even on x86_64
Note: With the current state of this PR everything compiles and it can built some kind of index. However there are still problems with SCANs which break most queries (at least on ARM, haven't tested ARM64 yet). Also
convertIndexWordToFloat()seems to be broken on 32 bit ARM.doesn't work on ARM. I guess the compiler tries to do it as an unary
minus before converting to off_t