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 muks):

 Hi Jinmei

 Replying to [comment:13 jinmei]:
 > 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;
 >     }
 > }}}

 This test is present because if `malloc()` does return NULL for some large
 size for example, the `allocated_size` variable doesn't become bogus. It
 means _we_ don't NULL test (to fail the allocate), but the caller _may_
 NULL test the returned pointer.

 > 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.

 When we are not able to allocate (by this, I don't mean large buffer
 requests that may fail, but smaller sizes which is unexpected), even if we
 handle this case, there's not much else to help the process then. It's an
 unreasonable condition for a typical process to continue beyond that point
 without requesting more from the data segment, even during cleanup (unless
 we just let it fail without any cleanup).

 It's not unreasonable to **not** handle an unexpected NULL return. There
 are people who do it and who don't (a matter of personal style from their
 experience). The effect being far away from cause is going to happen even
 if we test for NULL (over commiting policy).

 > 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.

 We are trying to find the best way for this. If you feel `allocate()`
 should throw, we'll add it. I don't mind both ways as I don't think either
 is less correct or makes a practical difference.

 > > 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.

 This becomes an issue because code that NULL tests libc returned pointers
 will be dead code, and termination happens randomly when the pages are
 used.

 > > 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);
 > }
 > }}}

 I didn't mean passing NULL to `free()`, but NULL to `deallocate()`
 regardless of its implementation.

 >
 > 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 you want it to be handled simply allowing NULLs (i.e, we allow NULL in
 `deallocate()` and simply decrement the size), the code does that.
 deallocate() should be documented for the NULL case so it's consistent.

 > > 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.

 I agree.. it's better to have this check than not have it.

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

 OK.

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