BIND 10 #2088: introduce MemorySegment class

[BIND 10 Development]

#2088: introduce MemorySegment class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120717
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 muks]:
 > Hi Jinmei
 >
 > Replying to [comment:9 jinmei]:
 > > Some quick comments:
 > >
 > > - I myself am not really sure, but I guess we'd probably like allocate
 > >   to throw when it fails to allocate the memory.
 >
 > "Fails to allocate memory" depends on the implementation, so whether it
 would throw or not would depend on the implementation.

 First, let me clarify I actually meant the case where malloc() returns
 NULL by "fail to allocate memory".

 > For the `malloc()` implementation, `malloc()` could return NULL. There
 are arguments both for and against NULL-testing this pointer, and I side
 with not NULL testing it unless we think it's reasonable for it to fail
 (such as for very large buffers). For small sizes if it returns NULL, in
 typical programs it is a fatal situation. Throwing here isn't likely to
 help and could have issues of its own. If large sizes are requested, then
 it's better left to the caller to handle a NULL return. For a well-written
 program running on a system reasonably configured for it, there should not
 be an unexpected out-of-memory issue in `malloc()`. If there is, it would
 be due to some programming (leak) or system configuration error.

 I tend to agree that when malloc() returns NULL for a reasonable size
 it would be leak or some system level error.  But for the rest of the
 above I either don't understand it or disagree on it.

 I don't know what you mean by issues of throwing itself, but our
 overall implementation generally expects when the free store operator
 'new' fails to allocate memory bad_alloc is thrown (although we
 generally only catch it at the highest level of the application and
 exit immediately).  Since MemorySegment::allocate() is a kind of
 wrapper for the standard memory allocator, I think it provides more
 consistent interface.

 I'm not sure what you mean by 'not NULL testing'.  The current
 implementation in the branch actually does test if the returned value
 is NULL or not:
 {{{#!cpp
     void* ptr = malloc(size);

     if (ptr != NULL) {
         allocated_size_ += size;
     }
 }}}

 If you mean by 'not NULL testing' we'd let the caller handle the
 situation where the pointer is NULL, I'd say it'll just result in the
 same behavior (throwing an exception) with forcing the caller to
 repeat the same pattern.  As noted above, our overall interface
 expects memory allocation to generally succeed or result in an
 exception in the worst case, so the caller would often have no other
 way to handle it than by throwing an exception itself (it's generally
 the case for constructors, and also apply to many methods/functions
 that return void).

 If you actually mean by 'not NULL testing' even the caller doesn't
 care if MemorySegment::allocate() returns NULL and it will let the
 program subsequently segfault or something, I've never heard of that
 argument as a reasonable design option.  At least I personally
 strongly oppose to it.  If the caller immediately tries to dereference
 the returned pointer, it will immediately cause a segfault, and the
 end result is not so different (I still prefer making it more explicit
 via an exception (with which we could handle it a bit more gracefully
 if we want) or assert()ing it, but at this level I see there are both
 for and against camps).  But if it simply stores the pointer somewhere
 and returns, basically assuming the pointer is valid, it will create a
 strange inconsistent state in the system, which will lead to very
 strange crash (if we are relatively lucky) or some awkward behavior
 (if we are more unlucky) that is very difficult to debug because the
 real cause of the problem is far from where the visible issue happens.

 So, to me, the only possible options were

 1. MemorySegment::allocate() throws if malloc() returns NULL, or
 2. MemorySegment::allocate() does nothing if malloc() returns NULL and
    passes it to caller.  We request the caller handle that situation
    appropriately (in documentation), which would be throwing an
    exception in practice anyway.

 Originally I was not sure which one is better in this specific case
 because in the limited usage of MemorySegment::allocate(), it might be
 more convenient for the caller to explicitly catch the rare failure
 case (than doing try-catch or using some wrapper class for RAII).  But
 I now expect it will probably not be the case.  Since option #1 is
 more consistent with other part of our internal interfaces, I now
 prefer this option.  But if you meant option #2 for some specific
 reason, we can discuss it.

 > There is one more issue. In some demand paging kernels (Linux included),
 allocations are over committed in the default configuration (unless it's a
 very very large buffer requested or some other conditions exist). For
 typical `malloc()`s, it will never return NULL. libc requests more memory
 and the kernel always allows the `brk()` to pass and creates table
 entries. Only when there's a page fault does the kernel try to find a
 frame for it and OOMs processes if necessary then. So NULL checking of
 `malloc()` output won't be useful.

 This doesn't seem to be an "issue" to me, just another incident of
 being locked in with a particular implementation.  The fact there's an
 implementation of malloc() that never returns NULL doesn't mean we
 "shouldn't" handle this error case.  And, there is in fact other
 implementation of malloc() in the real world that can really return
 NULL under memory shortage; I encountered that situation with BIND 9 -
 if my memory is correct it was Solaris.

 > NULL checking could be useful in other MemorySegment implementations.

 If we use boost memory segment libraries, they throw by default.
 Also, IMO the base class should provide the unified interface on this
 point anyway; we shouldn't change the behavior depending on details of
 the underlying segment implementation.

 > > - what if ptr is NULL in deallocate?  What if size is too large?
 >
 > Deallocating NULL would be a fatal programming mistake, not something
 that catching can fix. It's better for the process to segfault and dump
 its core then.

 First off, free() won't segfault with NULL.  So segfault won't
 happen with this implementation:
 {{{#!cpp
 void
 MemorySegmentLocal::deallocate(void* ptr, size_t size) {
     allocated_size_ -= size;
     free(ptr);
 }
 }}}

 Secondly, while I agree that deallocating attempt of NULL pointer is
 generally a program error (but not necessarily so - sometimes it could
 be a reasonable idiom for making the code simpler by eliminating the
 need for an if block), and I also agree that we cannot directly fix
 the error by catching it.  But it doesn't mean explicitly catching it
 doesn't make sense - it helps a possible bug earlier and more
 clearly.  I personally prefer such stricter checks and the benefit
 they provide, but I see this is a case where two camps exist.  Now
 Jelte seems to become the official reviewer of this branch, I'd leave
 it to you two.  In either case, I think the expected behavior should
 be documented.

 > If size is too large, we can probably add a trivial check against the
 allocated count. It will only catch larger than allocated count problems.
 It could be caught in the system's implementation if the implementation
 takes the size of memory to deallocate. Even in the case of `free()`, at
 the end we'd have a non-zero allocated count pointing to a bug. But we can
 add this check as it'd catch one type of problem.

 If size is too large, `allocated_size_` will become an obviously
 invalid, very large value.  I don't like to leave the object in such a
 clearly broken state.  Also, if this happens it's quite likely that
 caller is passing a pointer that was not really allocated by this
 segment.  IMO it's better to catch such an error as soon as possible.

 But I'd leave the decision on this point to you and Jelte, too.

 One other thing: the base class destructor should be declared and
 defined explicitly and be made virtual.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2088#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development