Skip to content
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

Native windows support #31

Closed
eiennohito opened this issue Sep 19, 2017 · 19 comments
Closed

Native windows support #31

eiennohito opened this issue Sep 19, 2017 · 19 comments

Comments

@eiennohito
Copy link
Contributor

eiennohito commented Sep 19, 2017

It should be possible to compile Juman++ on windows.
We need to port memory mapping and memory management utils to native Windows API for that.

@DoumanAsh
Copy link
Contributor

@eiennohito Does jumanpp continue to use boost? (I think first version used it)
It turns out there is mmap in boost http://www.boost.org/doc/libs/1_66_0/doc/html/boost/interprocess/mapped_region.html

@eiennohito
Copy link
Contributor Author

Nope, no boost, please. The Windows impl should be only around 60-70 lines of code with perfect error handling.

@DoumanAsh
Copy link
Contributor

Considering #20 I assume you would like to avoid strict dependency on C++17 std::aligned_alloc in place of posix_memalign?

@eiennohito
Copy link
Contributor Author

Yes, no C++17 for us (yet).
A lot of people use CentOS 7 or even 6 here.

Windows has https://msdn.microsoft.com/en-us/library/8z34s9c6.aspx, which should solve the problem.

@eiennohito
Copy link
Contributor Author

Even better idea would be to use https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx with actual large pages support, but I don't mind at least somehow working implementation. It's much better than nothing already.

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Jan 4, 2018

@eiennohito If we would go VirtualAlloc route, then wouldn't we also use linux's valloc? Not sure about its availability as it is part of glibc, not posix.

P.s. You cannot though specify alignment with VirtualAlloc, if it is needed.

@eiennohito
Copy link
Contributor Author

If you try to dig the history of memory.cpp, then you will find out that it was using mmap before! But the thing is, if you want to be a page to use huge tlb on Linux without defragmentation, you must align it on 2M boundary, and the best API to do that is posix_memalign.

Memory allocator itself should be ok with 4096/2M alignment which the VirtualAlloc will return.
It will handle smaller granularity by itself.

You should use _aligned_malloc for MallocEalloc (a bad name) nevertheless though.

@DoumanAsh
Copy link
Contributor

Now I hit quite strange problem with soa.h: https://github.com/ku-nlp/jumanpp/blob/master/src/util/soa.h#L50

It uses ManagedVector which is vector with your own custom allocator https://github.com/ku-nlp/jumanpp/blob/master/src/util/memory.hpp#L166-L192

But it seems this allocator misses custom copy constructor? To be honest MSVC is really bad with this error(I tried clang-cl though and it is not really helpful too)

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\vector(554): error C2664: 'jumanpp::util::memory::StlManagedAlloc<_Newfirst>::StlManagedAlloc(jumanpp::util::memory::StlManagedAlloc<_Newfirst> &&)': cannot convert argument 1 from 'jumanpp::util::memory::StlManagedAlloc<jumanpp::util::memory::SOAField *>' to 'jumanpp::util::memory::PoolAlloc *'
        with
        [
            _Newfirst=std::_Container_proxy
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\vector(554): note: No user-defined-conversion operator available that can perform this conversion, or the operator
cannot be called
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\vector(553): note: while compiling class template member function 'void std::_Vector_alloc<std::_Vec_base_types<_Ty,_Alloc>>::_Free_proxy(void)'
        with
        [
            _Ty=jumanpp::util::memory::SOAField *,
            _Alloc=jumanpp::util::memory::StlManagedAlloc<jumanpp::util::memory::SOAField *>
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\vector(505): note: see reference to function template instantiation 'void std::_Vector_alloc<std::_Vec_base_types<_Ty,_Alloc>>::_Free_proxy(void)' being compiled
        with
        [
            _Ty=jumanpp::util::memory::SOAField *,
            _Alloc=jumanpp::util::memory::StlManagedAlloc<jumanpp::util::memory::SOAField *>
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\vector(659): note: see reference to class template instantiation 'std::_Vector_alloc<std::_Vec_base_types<_Ty,_Alloc>>' being compiled
        with
        [
            _Ty=jumanpp::util::memory::SOAField *,
            _Alloc=jumanpp::util::memory::StlManagedAlloc<jumanpp::util::memory::SOAField *>
        ]
d:\repos\cpp-code\jumanpp\src\util\soa.h(50): note: see reference to class template instantiation 'std::vector<jumanpp::util::memory::SOAField *,jumanpp::util::memory::StlManagedAlloc<jumanpp::util::memory::SOAField *>>' being compiled

I'll try to poke around how custom allocators are supposed to be implemented, might take some time.

@eiennohito
Copy link
Contributor Author

Actually, there shouldn't be any use of soa.h inside the project now, it is an artifact of the past. If there is I will fix it.

@eiennohito eiennohito mentioned this issue Jan 7, 2018
@DoumanAsh
Copy link
Contributor

Ah yeah, it is not used anywhere. I guess soa.h and soa_test.cc can be removed

@eiennohito
Copy link
Contributor Author

Yep, to the better world should they go.

@DoumanAsh
Copy link
Contributor

I got interesting errors on my clang-cl:

D:\soft\Development\LLVM\bin\clang-cl.exe  /nologo -TP  -I..\libs -I..\src -Isrc\core\cfg /DWIN32 /D_WINDOWS /W3 /GR /EHsc /utf-8 /MDd /Zi /Ob0 /Od /RTC1 /showIncludes /Fosrc\core\CMakeFiles\jpp_core.dir\analysis\charlattice.cc.obj /Fdsrc\core\CMakeFiles\jpp_core.dir\jpp_core.pdb -c ..\src\core\analysis\charlattice.cc
..\src\core\analysis\charlattice.cc(284,19):  error: cannot pass object of non-trivial type 'std::_Vector_iterator<std::_Vector_val<std::_Simple_types<jumanpp::core::analysis::charlattice::TraverasalState *> > >' through variadic method; call will abort at runtime [-Wnon-pod-varargs]
    JPP_DCHECK_LT(begin, it);
                  ^
..\src\core\analysis\charlattice.cc(324,16):  note: in instantiation of function template specialization 'jumanpp::core::analysis::charlattice::unique<std::_Vector_iterator<std::_Vector_val<std::_Simple_types<jumanpp::core::analysis::charlattice::TraverasalState *> > >, (lambda at ..\src\core\analysis\charlattice.cc:325:23), std::vector<jumanpp::core::analysis::charlattice::TraverasalState *, jumanpp::util::memory::StlManagedAlloc<jumanpp::core::analysis::charlattice::TraverasalState *> > >' requested here
    auto ptr = unique(states1_.begin(), states1_.end(),
               ^
..\src\core\analysis\charlattice.cc(284,26):  error: cannot pass object of non-trivial type 'std::_Vector_iterator<std::_Vector_val<std::_Simple_types<jumanpp::core::analysis::charlattice::TraverasalState *> > >' through variadic method; call will abort at runtime [-Wnon-pod-varargs]
    JPP_DCHECK_LT(begin, it);
                         ^

It seems by default clang treats this warning as error.
I'm not sure if you saw something like that during your tests, I can add special flag to prevent error.

@eiennohito
Copy link
Contributor Author

I wonder what the warning/error means in the context. Need to dig a bit deeper, probably.

@DoumanAsh
Copy link
Contributor

@eiennohito I'm not sure of details but i assume it might be ABI issue. For now i decided to disable treating it as error.

@eiennohito
Copy link
Contributor Author

ABI compatibility for debug builds should not be a top priority. And these macros are a noop in debug builds.

Or maybe we should take parameters inside the assert macros by const&.

eiennohito added a commit that referenced this issue Jan 10, 2018
should fix warnings described in #31
@eiennohito
Copy link
Contributor Author

Should fix those warnings at least for assert stuff.
If there are more places, tell me where!

@eiennohito
Copy link
Contributor Author

When #74 and #76 are closed, this issue will become a history! Yay!

@DoumanAsh
Copy link
Contributor

Seems like it can be closed? Congrats on getting windows support complete!
We only need to update README and put appveyor badge :)

@eiennohito
Copy link
Contributor Author

Yep, I wanna do a rc2 release with the readme update and stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants