Add 64 bit support to FastPFor #58

xndai · 2019-06-20T22:45:52Z

(try to reopen the PR and fix the Windows build issue. The commit history seems to be messed up. I am not sure why...)

Add support for 64-bit integers for FastPFor

1. Add a new template parameter for FastPFor encoder to specify the integer type. Curently uint32_t and uint64_t are supported.
2. Add corresponding bit packing and unpacking functions for 64-bit integers.
3. Overload IntegerCODEC interfaces to provide 64bit version of encodeArray() and decodeArray(). For codes that don't support 64bit, those would just throw not implemented exception.
4. Add 64-bit support for CompositeCodec and VariableByte, so the encoding and decoding can work end to end with arbitary number of integers.
5. Add gtest to the CMake project and create unittests for the new 64 bit support.
6. Replace some of the assertions into exceptions.
7. Fix issues with Windows build according to feedback from @pps83

- codecs.cpp and inmemorybenchmark.cpp already contain non-LGPL inline version of getopt - fixed warning (trailing semicolon in pragma statement in inline getopt code) - define NOMINMAX to make sure that windows.h doesn't define min/max

…ing.cpp and simdunalignedbitpacking.cpp

- enable AVX in msvc builds to enable ssse4 and sse4.1 - modify __SSSE3__/__SSE4_1__ checks and warnings pragmas to support msvc builds

1. Add a new template parameter for FastPFor encoder to specify the integer type. Curently uint32_t and uint64_t are supported. 2. Add corresponding bit packing and unpacking functions for 64-bit integers. 3. Overload IntegerCODEC interfaces to provide 64bit version of encodeArray() and decodeArray(). For codes that don't support 64bit, those would just throw not implemented exception. 4. Add 64-bit support for CompositeCodec and VariableByte, so the encoding and decoding can work end to end with arbitary number of integers. 5. Add gtest to the CMake project and create unittests for the new 64 bit support. 6. Replace some of the assertions into exceptions.

@pps83

1. Add a new template parameter for FastPFor encoder to specify the integer type. Curently uint32_t and uint64_t are supported. 2. Add corresponding bit packing and unpacking functions for 64-bit integers. 3. Overload IntegerCODEC interfaces to provide 64bit version of encodeArray() and decodeArray(). For codes that don't support 64bit, those would just throw not implemented exception. 4. Add 64-bit support for CompositeCodec and VariableByte, so the encoding and decoding can work end to end with arbitary number of integers. 5. Add gtest to the CMake project and create unittests for the new 64 bit support. 6. Replace some of the assertions into exceptions. 7. Fix issues with Windows build according to feedback from @pps83

lemire · 2019-06-20T22:47:20Z

@pps83 Would you check this PR?

xndai · 2019-06-20T22:54:08Z

It's weird that the commit looks normal in my forked repo, but after opening a pull request, it includes some previous commits which shouldn't be there. Any idea? @lemire @pps83

$ git show --pretty="" --name-only 38c332e
CMakeLists.txt
CMakeLists.txt.in
headers/bitpacking.h
headers/bitpackinghelpers.h
headers/blockpacking.h
headers/codecs.h
headers/compositecodec.h
headers/fastpfor.h
headers/newpfor.h
headers/packingvectors.h
headers/pfor.h
headers/pfor2008.h
headers/simdbinarypacking.h
headers/simdfastpfor.h
headers/simdgroupsimple.h
headers/simdnewpfor.h
headers/simdpfor.h
headers/util.h
headers/variablebyte.h
src/bitpacking.cpp
unittest/test_composite.cpp
unittest/test_fastpfor.cpp
unittest/test_variablebyte.cpp

lemire · 2019-06-21T00:39:57Z

You mean that there are commits that you do not see there...
https://github.com/xndai/FastPFor/commits/fastpfor64_support

xndai · 2019-06-21T17:18:32Z

No, I mean in this particular PR, if you look at the diff, there are not just the 64bit support changes I add, but also includes previous changes from you and @pps83

xndai · 2019-06-21T18:38:23Z

Now the Windows build has passed. Can somebody help test it out? Thanks.

lemire · 2019-06-21T19:20:22Z

No, I mean in this particular PR, if you look at the diff, there are not just the 64bit support changes I add, but also includes previous changes from you and @pps83

I see it now. It is weird.

lemire · 2019-06-21T19:20:47Z

Ping @pps83 ... can you provide feedback?

pps83 · 2019-06-22T11:16:13Z

@xndai I tested with https://github.com/xndai/FastPFor/tree/fastpfor64_support

I think your original test_driver should be restored :) VS doesn't seem to like to link to gtest_main (it doesn't find entry point).
So, you should just link to gtest only, and add test_main.cpp:

#include "gtest/gtest.h"

int main(int argc, char **argv)
{
    testing::InitGoogleTest(&argc, argv);
    return RUN_ALL_TESTS();
}

I think you don't need anything from mock, as you use only gtest (you should remove all references to gmock in cmake files and include paths). Not sure if you still have it, I recall seeing it in the original PR.
I don't know what was changed, but test_composite.cpp doesn't compile now as-is, because it fails somewhere in std headers because it brings c++17 requirement. When I enable c++17 in unit test project it compiles though. maybe you can find what resulted in that requirement and update the code.
after (1), (2) and (3) I get these tests failed:

[  FAILED  ] 12 tests, listed below:
[  FAILED  ] FastPForLib/FastPForTest.increasingSequence/0, where GetParam() = "FastPFor128"
[  FAILED  ] FastPForLib/FastPForTest.increasingSequence/1, where GetParam() = "FastPFor256"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_zeros_with_exceptions/0, where GetParam() = "FastPFor128"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_zeros_with_exceptions/1, where GetParam() = "FastPFor256"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_5_except/0, where GetParam() = "FastPFor128"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_5_except/1, where GetParam() = "FastPFor256"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_7_except_63/0, where GetParam() = "FastPFor128"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_7_except_63/1, where GetParam() = "FastPFor256"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_0_except_32/0, where GetParam() = "FastPFor128"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_0_except_32/1, where GetParam() = "FastPFor256"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_3_except_64/0, where GetParam() = "FastPFor128"
[  FAILED  ] FastPForLib/FastPForTest.fastpack_3_except_64/1, where GetParam() = "FastPFor256"

12 FAILED TESTS

some fail hard errors:

and some fail with exceptions thrown:

etc.

pps83 · 2019-06-22T11:19:55Z

Also, note I did not look through the code/changes at all this time, but when test_composite.cpp failed to compile in (3) I noticed that you do std::srand(std::time(nullptr));
First of all, srand seems pointless there, as the code uses random_device. Second, if you use random_device the test may fail on some random input ... and then what are you going to do if you don't even know what kind of output triggered failed test? So, you really need to use some reproducible generator (eg you cannot use random device), and if a test fails, then you need to log what seed triggered it with a strong message to whoever got it to create a github issue with that seed :)
Also it uses int64_t throughout, but I thought that compressor expects uint64_t?

lemire · 2019-06-22T14:46:26Z

Important: tests should be deterministic. You do not want randomness. (Pseudo-randomness with a fixed seed is fine, as long as you keep the random generator the same on all platforms.)

1. Add back test driver cpp file, and link to gtest instaed of gtest_main 2. Add logging for the input array when test fails 3. Remove unnecessary srand() calls.

lemire · 2019-06-24T21:22:18Z

Pinging @pps83

xndai · 2019-06-24T21:25:21Z

@pps83 I've updated the PR. Please take a look.

Some answers to your comments -

I am not sure why test_composite.cpp triggered build failure. The only difference I have is removing two commented lines. Besides the AppVeyor build did pass. I think it's important to set up gating build in a way that such issue won't slip through.
Regarding the gmock, I don't use it currently in the project. But I keep them in the cmake file just because this is an official cmake template to use.
Regarding the SEH exception, do you have more information for debugging? A call stack? I don't have a way to diagnose and repro. I see this stack overflow answer regarding SEH exception with gtest. Maybe you want to give it a try? https://stackoverflow.com/questions/13157671/seh-exception-with-code-0xc0000005-thrown-in-the-test-body
I add the logging for failed test case, so even if the input is randomly generated, we could still repro and debug. Also remove the unnecessary srand() calls. Thanks for pointing that out.

xndai · 2019-06-24T21:34:36Z

Also I tested on MacOs (clang) and Linux (gcc). All test passed.

lemire · 2019-06-25T00:39:25Z

@xndai It would be best to test using only fixed seeds? Testing with a randomly generated seed is a usability issue. You may be logging the failing tests in a way that you can reproduce the issue (yet is the output of "rd()" really logged?)... but we want everyone to be able to run the tests and identify the issues. With random seeds, other people are likely to suffer. And it super easy to test with a fixed seed. E.g., it suffices to replace "std::mt19937_64 e2(rd());" with "std::mt19937_64 e2(12345);". There is really no benefit to genuinely random testing... unless maybe if you are fuzzing and even then...

pps83 · 2019-06-25T16:05:49Z

3. Regarding the SEH exception, do you have more information for debugging? A call stack? I don't have a way to diagnose and repro. I see this stack overflow answer regarding SEH exception with gtest.

SEH exception basically means program crash.
Here's backtrace:

 	[Inline Frame] FastPFor_unittest.exe!?A0x5f4e5c16::unpack_single_out(const unsigned int * &) Line 33	C++
>	[Inline Frame] FastPFor_unittest.exe!`anonymous-namespace'::Unroller<59,0>::Unpack(const unsigned int * &) Line 131	C++
 	FastPFor_unittest.exe!__fastunpack59(const unsigned int * in, unsigned __int64 * out) Line 712	C++
 	FastPFor_unittest.exe!FastPForLib::packingvector<32>::unpackmetight<unsigned __int64>(const unsigned int * in, unsigned __int64 * out, unsigned __int64 outSize, const unsigned int bit) Line 42	C++
 	FastPFor_unittest.exe!FastPForLib::FastPForImpl<4,unsigned __int64>::__decodeArray(const unsigned int * in, unsigned __int64 & length, unsigned __int64 * out, const unsigned __int64 nvalue) Line 239	C++
 	[Inline Frame] FastPFor_unittest.exe!FastPForLib::FastPForImpl<4,unsigned __int64>::decodeArray(const unsigned int * in, const unsigned __int64 out, unsigned __int64 *) Line 87	C++
 	FastPFor_unittest.exe!FastPForLib::FastPFor<4>::decodeArray(const unsigned int * in, const unsigned __int64 length, unsigned __int64 * out, unsigned __int64 & nvalue) Line 303	C++
 	FastPFor_unittest.exe!FastPForLib::FastPForTest::_verify64() Line 90	C++

Overall, something really bad happens in __decodeArray. These are local values:

Note that nvalue is 4294967295. (or 0xFFFFFFFF), because of that (nvalue + PACKSIZE - 1) / PACKSIZE * PACKSIZE results in overflow and size is 0. Then calling datatobepacked[k].data() on that empty vector results in nullptr (vector::data is free to return nullptr if vector is empty).

xndai · 2019-06-25T17:31:47Z

@pps83 if possible, can you step in decodeArray() and see how "thissize" is calculated?

Also can we add one more step in our Windows gating build to run FastPFor_unittest? I want to make sure we are able to repro this in an official build environment.

I also plan to borrow a Windows machine for debugging. Did you use "cmake ." and "make" command to produce the windows binaries?

lemire · 2019-07-03T12:26:37Z

@xndai

We are getting further but there are still issues. Can you have a look at the appveyor logs and see what can be done?

xndai · 2019-07-03T17:18:55Z

@lemire I think right now the last blocker is at the external vector.h line 184. random_shuffle() is deprecated by c++ 17. But using shuffle() with a random device instead would trigger compiler error somewhere in the random header file. If I just comment out line 184 and 188, I can get a build pass locally on my VS2019.

pps83 · 2019-07-03T18:55:33Z

I tested latest xndai:fastpfor64_support and all tests pass now.

lemire · 2019-07-03T19:04:56Z

I am puzzled as to why the tests do not pass on appveyor.

xndai · 2019-07-03T19:07:15Z

The appveyor doesn't even get a successful build. Can we go back to the previous build settings? At least we got successful build at the beginning.

lemire · 2019-07-03T20:07:32Z

C++ 17 is not fully supported Visual Studio 2017. That seems to be the problem. We are flipping what is effectively an experimental flag.

The solution, I think, is to simply not rely on C++17. C++14 is fully supported.

Why must we require C++17? That seems like an orthogonal issue to the object of your PR. Is there any reason why C++14 is not enough?

Is there any chance you might simply revert to C++14?

Can we go back to the previous build settings?

You mean appveyor?

If you have a proposal for new appveyor settings, settings that build and run tests, please share them. Here are the current settings...

https://github.com/lemire/FastPFor/blob/master/appveyor.yml

Note that the tests run fine with master...

Merging without CI tests is not a good idea, I am sure you will agree.

pps83 · 2019-07-04T04:37:24Z

Why must we require C++17? That seems like an orthogonal issue to the object of your PR. Is there any reason why C++14 is not enough?

There is no need for any C++XX configs. Just remove #define constexpr inline @xndai and it will build and run. More over, that define was bad anyways, as it lives in a public header and will break other projects in unexpected mysterious ways (it does break if you include windows.h after fastpfor headers).

Also, you may as well change #define __restrict__ -> #define __restrict__ __restrict.
This will work in vs2015/2017 (I don't have older VS installs to try).

Regarding gtest: I've seen projects simply copy gtest folder and use it. Much simpler that way, also you'll be able to add VS projects and it will just work in VS without doing anything extra to install gtest.

lemire · 2019-07-04T12:51:31Z

@pps83

Great catch!

I don't think these defines come from this PR. They go back to an era when they were a workaround. And they are not the primary reason for the failure here since they are in master, and master pass our tests on appveyor. But it could be the final/ultimate cause of our current disaster.

@xndai Still... do you want to try to remove the C++17 flags while removing the defines as suggested by @pps83. And see where it takes us... I am hopeful that it should help.

To be clear... @xndai is not the one who wrote these defines... but he may be the first one impacted by them.

lemire · 2019-07-04T13:07:11Z

I have created a distinct PR

#60

lemire · 2019-07-04T13:07:30Z

Let us see if the new PR passes our tests. If it does it might help here as well.

lemire · 2019-07-04T13:48:23Z

@pps83

Ok. Removing these defines breaks the Visual Studio build...

#60

So they are there for a reason. We can't "just" remove the defines.

lemire · 2019-07-04T14:09:11Z

@pps83 Trying to require C++11 support. Hopefully this will get Visual Studio to accept constexpr.

lemire · 2019-07-04T14:16:47Z

That seems to have done the trick. I am letting the tests run, and then I will merge this other PR. It may help with the current one.

I still think we should not need C++17.

lemire · 2019-07-04T15:49:13Z

Relaunching the tests. We no longer redefine constexpr.

lemire · 2019-07-04T16:40:02Z

Ah. The tests are running... the build was a success.

xndai · 2019-07-04T16:50:17Z

Wonderful. Are you able to add FastPFor_unittest as part of the post build testing?

lemire · 2019-07-04T16:53:49Z

@xndai

You modified the CMake build so that it includes FastPFor_unittest in the tests, right? If so, it should run... but you are right that we should double-check the logs to see that it is there.

Let us wait for the tests to finish on appveyor.

lemire · 2019-07-04T19:19:27Z

Everything is green. I'll merge and see about reverting the C++17 flag.

pps83 · 2019-07-05T11:21:24Z

I use vs2017, I think vs2017 by default enables c++11, so in my case without setting any c++XX version everything builds and runs as expected. Perhaps for vs2015 c++11 needs to be enabled, no need to require c++17

lemire · 2019-07-05T12:11:06Z

We require C++11 as part of the CMake build, which should translate into the proper requirements and flags for Visual Studio when build via CMake.

xndai · 2019-07-07T04:18:32Z

Awesome! Thanks everyone's help on this.

lemire · 2019-07-07T12:35:56Z

See https://github.com/lemire/FastPFor/blob/master/AUTHORS

amallia and others added 11 commits June 20, 2019 14:32

Update compiler id check

ab52361

Update simple16.h

d988174

Add .gitignore and .gitattributes

4b43e9b

Remove unneeded getopt from msvc projects

e24924f

- codecs.cpp and inmemorybenchmark.cpp already contain non-LGPL inline version of getopt - fixed warning (trailing semicolon in pragma statement in inline getopt code) - define NOMINMAX to make sure that windows.h doesn't define min/max

Move similarly named functions to different namespaces in simdbitpack…

7ce9467

…ing.cpp and simdunalignedbitpacking.cpp

Port maskedvbyte/streamvbyte to Visual Studio

d993230

Port varintgb to Visual Studio

a3d682c

- enable AVX in msvc builds to enable ssse4 and sse4.1 - modify __SSSE3__/__SSE4_1__ checks and warnings pragmas to support msvc builds

Fix build break on MSVC due to constexpr is not supported

1896195

Revert "Add support for 64-bit integers for FastPFor"

d9a21c6

xndai added 2 commits June 21, 2019 10:43

Fix Windows build failure

05475b3

Fix Windows build failure

c1e7640

Update change according to feedback from @pps83

ac1c13d

1. Add back test driver cpp file, and link to gtest instaed of gtest_main 2. Add logging for the input array when test fails 3. Remove unnecessary srand() calls.

xndai added 4 commits July 2, 2019 20:59

Use _BitScanReverse() for 64 bits under WIN32

6686d69

Use _BitScanReverse() for 64 bits under WIN32

d5acc99

Fix build error - rename local variable

80042fd

Use master branch for gtest

04b8579

xndai added 2 commits July 3, 2019 09:41

Enable c++ 17 for MSVC

92cbcfe

Remove std::random_shuffle() as it's deprecated by c++ 17

055f0e1

lemire merged commit 864713d into fast-pack:master Jul 4, 2019

lemire mentioned this pull request Jul 4, 2019

Add support for 64-bit integers #23

Closed

Add 64 bit support to FastPFor #58

Add 64 bit support to FastPFor #58

Uh oh!

Conversation

xndai commented Jun 20, 2019

Uh oh!

lemire commented Jun 20, 2019

Uh oh!

xndai commented Jun 20, 2019

Uh oh!

lemire commented Jun 21, 2019

Uh oh!

xndai commented Jun 21, 2019

Uh oh!

xndai commented Jun 21, 2019

Uh oh!

lemire commented Jun 21, 2019

Uh oh!

lemire commented Jun 21, 2019

Uh oh!

pps83 commented Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pps83 commented Jun 22, 2019

Uh oh!

lemire commented Jun 22, 2019

Uh oh!

lemire commented Jun 24, 2019

Uh oh!

xndai commented Jun 24, 2019

Uh oh!

xndai commented Jun 24, 2019

Uh oh!

lemire commented Jun 25, 2019

Uh oh!

pps83 commented Jun 25, 2019

Uh oh!

xndai commented Jun 25, 2019

Uh oh!

lemire commented Jul 3, 2019

Uh oh!

xndai commented Jul 3, 2019

Uh oh!

pps83 commented Jul 3, 2019

Uh oh!

lemire commented Jul 3, 2019

Uh oh!

xndai commented Jul 3, 2019

Uh oh!

lemire commented Jul 3, 2019

Uh oh!

pps83 commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemire commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

xndai commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

lemire commented Jul 4, 2019

Uh oh!

pps83 commented Jul 5, 2019

Uh oh!

lemire commented Jul 5, 2019

pps83 commented Jun 22, 2019 •

edited

Loading

pps83 commented Jul 4, 2019 •

edited

Loading

lemire commented Jul 4, 2019 •

edited

Loading