Master doesn't compile

[bluechill]

Master doesn't compile on OS X.

soar_memory_pool_allocator() {}

is a private constructor which needs to be non-private for OS X stdlib. If you make it public it compiles and passes the tests but is this an appropriate fix?

@mazina @bazald

[justinnhli]

Confirmed. Compiler output:

scons: Reading SConscript files ...
Building intermediates to build
Installing targets to /Users/justinnhli/Soar/out
Tcl headers and libraries not found at /Library/Frameworks/Tcl.framework/Versions/Current, not building Tcl SML wrappers.
C# compiler not found, not building C# SML wrappers
Tcl headers and libraries not found at /Library/Frameworks/Tcl.framework/Versions/Current, not building TclSoarLib.
scons: done reading SConscript files.
scons: Building targets ...
Making build/Core/SoarKernel/SoarKernel.os
In file included from Core/SoarKernel/SoarKernel.cxx:1:
In file included from Core/SoarKernel/src/agent.cpp:23:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/map:423:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tree:16:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1954:40: error: field of type
      'soar_module::soar_memory_pool_allocator<std::__1::__list_node<production_struct *, void *> >' has private default constructor
    _LIBCPP_INLINE_VISIBILITY explicit __libcpp_compressed_pair_imp(_T1_param __t1)
                                       ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2317:11: note: in instantiation of member function
      'std::__1::__libcpp_compressed_pair_imp<unsigned long, soar_module::soar_memory_pool_allocator<std::__1::__list_node<production_struct *, void *> >,
      0>::__libcpp_compressed_pair_imp' requested here
        : base(_VSTD::forward<_T1_param>(__t1)) {}
          ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/list:683:7: note: in instantiation of member function
      'std::__1::__compressed_pair<unsigned long, soar_module::soar_memory_pool_allocator<std::__1::__list_node<production_struct *, void *> > >::__compressed_pair' requested here
    : __size_alloc_(0)
      ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/list:831:5: note: in instantiation of member function
      'std::__1::__list_imp<production_struct *, soar_module::soar_memory_pool_allocator<production_struct *> >::__list_imp' requested here
    list()
    ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/list:2057:23: note: in instantiation of member function
      'std::__1::list<production_struct *, soar_module::soar_memory_pool_allocator<production_struct *> >::list' requested here
    list<_Tp, _Alloc> __deleted_nodes; // collect the nodes we're removing
                      ^
Core/SoarKernel/src/reinforcement_learning.cpp:254:9: note: in instantiation of member function 'std::__1::list<production_struct *,
      soar_module::soar_memory_pool_allocator<production_struct *> >::remove' requested here
        rules->remove( prod );
               ^
Core/SoarKernel/src/soar_module.h:1182:3: note: declared private here
                soar_memory_pool_allocator() {}
                ^
1 error generated.
scons: *** [build/Core/SoarKernel/SoarKernel.os] Error 1
scons: building terminated because of errors.

[mazina]

I can confirm this happens on the 9.5 branch as well.

I'm pretty sure Nate wrote the mempool allocator code, and he's a mac man. And I know at least Aaron has been actively working on and compiling SVS, though I don't know if he or anyone has been doing it on a mac. Perhaps one of them has a suggestion... @natederbinsky @amininger

What's weird is that is that the master branch hasn't really changed since Oct 3rd, and Jenkins shows that it compiled and passed unit tests then on all platforms. (I tried the commit from back then, and it now gets this error too.) So, perhaps it has something to do with a change in the Mac libraries since then?

[bluechill]

It probably has something to do with OS X libraries being updated. But it's still an error we should fix. I talked with @natederbinsky briefly and he didn't really offer anything to make it work or not (he also didn't really look into this issue as well).

So long as we can ensure that an default allocated soar allocator is never referenced in Soar code (because it's agent will be null) then I think it's fine to make it public. I do not know if this would be the case or not. It probably will be fine but someone should make sure it's never referenced before saying yes or not.

[mazina]

I retract what I said about SVS. I'm sorry, SVS. Anyway, it wasn't happening in my 9.5 setup because I had USE_MEM_POOL_ALLOCATORS disabled to make debugging memory issues easier, so I guess this issue is universal.

I don't know the answer to your question, Alex. I've never had to work with those. Is that something you can look for in the code?

[mazina]

If it was private before, doesn't that guarantee nothing else called the allocator that didn't have access to it before?

[justinnhli]

It actually seems like the standard libraries changed such that remove() requires the default constructor. I can't for the life of me think of why though.

[bazald]

A private default constructor for a custom memory allocator seems pretty odd. I was under the impression that they got called all the time. If we don't actually need allocations from default-constructed custom allocators, commit b2b32115b3ed90481408c36268d8701e363ea1b2 ought to resolve the issue. If not, assertions will result.

[bazald]

Are there good reasons for memory pools to be agent specific? We could fix the custom allocator properly if we decoupled memory pools from agents and just had one globally accessible set of memory pools.

[mazina]

I don't know of any good reason. Perhaps it's an artifact of the conversion from single-agent Soar, when it seems like everything was put into the agent struct by default to simplify the conversion.

For what it's worth, I couldn't find any cases of the most common bad reason I've seen in other parts of the code, namely being able to call print() for debug purposes.

[bazald]

If commit b2b32115b3ed90481408c36268d8701e363ea1b2 resolves this issue (can someone who experienced it test?), I suggest closing it, making a new issue for decoupling the memory pools from the agents, and eventually enabling custom memory allocator functionality for default-constructed allocator objects once that's done.

[bluechill]

b2b3211 resolves this issue. b2b3211 passes all Unit Tests.

[marinier]

@bazald FYI, a possible reason to not share memory pools across agents is if we ever want to support running them in separate threads (like jsoar). Presumably it will be easier and more efficient if each agent has it's own memory pools.

[bazald]

@marinier If that's the goal, I still think decoupling memory pools from the agent code is the way to go. Doing thread-local storage properly seems like the right way to provide different memory pools to different threads.

[marinier]

@bazald Agreed. I'm not opposed to decoupling at all. I was just raising a possible concern about sharing the decoupled thing across agents.

[garfieldnate]

Is it not also necessary for providing memory usage stats of individual agents?

[marinier]

Also an excellent point. I suppose there might be another way to track this if the memory were shared, but it would be more complex/less efficient than the current method.

On Fri, May 8, 2015 at 5:32 AM, Nathan Glenn notifications@github.com wrote:

Is it not also necessary for providing memory usage stats of individual agents?

— Reply to this email directly or view it on GitHub https://github.com/SoarGroup/Soar/issues/217#issuecomment-100172909.

[garfieldnate]

If someone had the time to think through a new design and implement and debug it, it could very useful. There's a lot of global state and compile-time dependencies that could be resolved with a refactor.

[marinier]

JSoar is the refactored version. Ideally, that would just be ported to CSoar.

On Fri, May 8, 2015 at 10:00 AM, Nathan Glenn notifications@github.com wrote:

If someone had the time to think through a new design and implement and debug it, it could very useful. There's a lot of global state and compile-time dependencies that could be resolved with a refactor.

— Reply to this email directly or view it on GitHub https://github.com/SoarGroup/Soar/issues/217#issuecomment-100240611.

[garfieldnate]

JSoar has the Java GC, though. Does it still manage it's own memory?

[garfieldnate]

Are there any comparisons around for the speed of JSoar and CSoar?

[marinier]

It doesn't have it's own memory pools, but it does use reference counts in some places (a subset of where csoar uses them; I can explain why if you're interested). But I was just saying that jsoar has already eliminated all global variables, so rather than re-solve that problem in a different way that makes the jsoar and csoar code get (more) out-of-sync, it would be better to take the opportunity to bring the code bases closer together.

On Sat, May 9, 2015 at 12:53 AM, Nathan Glenn notifications@github.com wrote:

JSoar has the Java GC, though. Does it still manage it's own memory?

— Reply to this email directly or view it on GitHub https://github.com/SoarGroup/Soar/issues/217#issuecomment-100428256.

[marinier]

Yes, there have been several attempts to compare; there have been talks in at least a couple previous workshops.

Also, jsoar includes the jsoar-performance-testing project (written by Alex T) that allows you to run a series of tests on jsoar (any recent version) and csoar (any 64-bit version since 9.0, although not yet tested on 9.5). In past testing, csoar is 2-5x faster, depending on the test. jsoar also uses significantly more memory. But development in jsoar is 10x faster and integration with java projects is far easier, so that's the tradeoff. At SoarTech, we end up using jsoar about 90% of the time.

On Sun, May 10, 2015 at 8:57 AM, Nathan Glenn notifications@github.com wrote:

Are there any comparisons around for the speed of JSoar and CSoar?

— Reply to this email directly or view it on GitHub https://github.com/SoarGroup/Soar/issues/217#issuecomment-100638110.

[mazina]

@bazald @bluechill Hey guys, I've found a case where your new asserts have fired. If memory allocators are on, the unit test FullTests::testCommandToFile will crash. The deallocator gets called, but mempool and agent are both null. When it happens, it is excising an RL production so that it could source one with the same name. So, it seemed to have something to do with the RL memory allocators. I didn't notice this at first because that test was disabled. Comment out SKIP_SLOW_TESTS in unittests.h to enable.

[bazald]

Should I have to do something besides commenting out SKIP_SLOW_TESTS in unittest.h? It just passed all tests, including the one you mentioned, on my gq-lambda branch, and I can't imagine I fixed your issue.

[mazina]

The only other things is turning on mempools and mempool allocators in kernel.h

[mazina]

I tried 9.4.0 and 9.3.4. They both die on that test with mempool allocators on, and I'm pretty sure they didn't back when they were released. Is this issue mac specific or does the change to public open up this problem on other platforms?

[bazald]

I just tried it on tags/SoarSuite-9.4.0 and it still worked fine for me on LMDE x64.

[bluechill]

Remember @bazald while both Linux and OS X are compatible in C++ from a high level, clang and libc++ are more strict with the standard than most standard libraries and compilers. They will even tell you such on their website. So it could be clang is generating stricter code or libc++ is calling stuff more strictly. I will try it on my laptop when I get a chance today.

[bazald]

@bluechill, clang is cross-platform. I was using clang in the first place. I just tried to force it to use libc++abi and ran into too much trouble. That's what I get for blowing away the version I compiled myself and trying to use the apt-provided copy, I suppose.

[mazina]

Just an update. Mitchell and I looked at it yesterday and could not come up with a good solution or even an explanation that includes why this issue isn't widespread and making everything fail. It may be time to pull the father of these allocators in, @natederbinsky...

[bluechill]

Are we considering this fixed?

[mazina]

Yes. I added a memory manager that decoupled the memory code from the agent. It's ok with allocating and deallocating those weird instances being created.