Skip to content
Snippets Groups Projects
Forked from Alchemy Viewer / Alchemy Viewer
Source project has a limited visibility.
  • Nat Goodspeed's avatar
    2902f23a
    DRTVWR-476: Remove special llcorehttp test memory manager. · 2902f23a
    Nat Goodspeed authored
    NickyD discovered that the substitute default allocator used for llcorehttp
    tests was returning badly-aligned storage, which caused access violations on
    alignment-sensitive data such as std::atomic. Thanks Nicky!!
    
    Moreover, the llcorehttp test assertions regarding memory usage, well-
    intentioned though they are, have been causing us trouble for years. Many have
    already been disabled.
    
    The problem is that use of test_allocator.h affected *everything* defined with
    that header file's declarations visible. That inevitably included specific
    functions in other subsystems. Those functions then (unintentionally) consumed
    the special allocator, throwing off the memory tracking and making certain
    memory-related assertions consistently fail.
    
    This is a particular, observable bad effect of One Definition Rule violations.
    Within a given program, C++ allows multiple definitions for the same entity,
    but requires that all such definitions be the same. Partial visibility of the
    global operator new() and operator delete() overrides meant that some
    definitions of certain entities used the default global allocator, some used
    llcorehttp's. There may have been other, more subtle bad effects of these ODR
    violations.
    
    If one wanted to reimplement verification of the memory consumption of
    llcorehttp classes:
    
    * Each llcorehttp class (for which memory tracking was desired) should declare
      class-specific operator new() and operator delete() methods. Naturally,
      these would all consume a central llcorehttp-specific allocator, but that
      allocator should *not* be named global operator new().
    * Presumably that would require runtime indirection to allow using the default
      allocator in production while substituting the special allocator for tests.
    * Recording and verifying the memory consumption in each test should be
      performed in the test-object constructor and destructor, rather than being
      sprinkled throughout the test<n>() methods.
    * With that mechanism in place, the test object should provide methods to
      adjust (or entirely disable) memory verification for a particular test.
    * The test object should also provide a "yes, we're still consuming llcorehttp
      memory" method to be used for spot checks in the middle of tests -- instead
      of sprinkling in explicit comparisons as before.
    * In fact, the llcorehttp test object in each test_*.hpp file should be
      derived from a central llcorehttp test-object base class providing those
      methods.
    2902f23a
    History
    DRTVWR-476: Remove special llcorehttp test memory manager.
    Nat Goodspeed authored
    NickyD discovered that the substitute default allocator used for llcorehttp
    tests was returning badly-aligned storage, which caused access violations on
    alignment-sensitive data such as std::atomic. Thanks Nicky!!
    
    Moreover, the llcorehttp test assertions regarding memory usage, well-
    intentioned though they are, have been causing us trouble for years. Many have
    already been disabled.
    
    The problem is that use of test_allocator.h affected *everything* defined with
    that header file's declarations visible. That inevitably included specific
    functions in other subsystems. Those functions then (unintentionally) consumed
    the special allocator, throwing off the memory tracking and making certain
    memory-related assertions consistently fail.
    
    This is a particular, observable bad effect of One Definition Rule violations.
    Within a given program, C++ allows multiple definitions for the same entity,
    but requires that all such definitions be the same. Partial visibility of the
    global operator new() and operator delete() overrides meant that some
    definitions of certain entities used the default global allocator, some used
    llcorehttp's. There may have been other, more subtle bad effects of these ODR
    violations.
    
    If one wanted to reimplement verification of the memory consumption of
    llcorehttp classes:
    
    * Each llcorehttp class (for which memory tracking was desired) should declare
      class-specific operator new() and operator delete() methods. Naturally,
      these would all consume a central llcorehttp-specific allocator, but that
      allocator should *not* be named global operator new().
    * Presumably that would require runtime indirection to allow using the default
      allocator in production while substituting the special allocator for tests.
    * Recording and verifying the memory consumption in each test should be
      performed in the test-object constructor and destructor, rather than being
      sprinkled throughout the test<n>() methods.
    * With that mechanism in place, the test object should provide methods to
      adjust (or entirely disable) memory verification for a particular test.
    * The test object should also provide a "yes, we're still consuming llcorehttp
      memory" method to be used for spot checks in the middle of tests -- instead
      of sprinkling in explicit comparisons as before.
    * In fact, the llcorehttp test object in each test_*.hpp file should be
      derived from a central llcorehttp test-object base class providing those
      methods.
Code owners
Assign users and groups as approvers for specific file changes. Learn more.