allocators are being ignored on different constructors

[kostasrim]

Hi there,

Allocators are not properly propagated on constructors. Two cases I found (not exhaustively):

//case A
basic_json(const Allocator&)
{
    construct<empty_object_storage>(semantic_tag::none);
}

//case B
basic_json(const basic_json& other, const Allocator& alloc)
{
    uninitialized_copy_a(other,alloc); // ignores `alloc` :(
}

Both cases (A, B) above ignore alloc

std::pmr::polymorphic_allocator is a stateless allocator. However, the way pmr works is the argument to it's constructor std::memory_resource is a polymorphic object. So when a user wants to supply a different allocator (let's say jemalloc) they will inherit from memory_resource and override the methods do_allocate, do_deallocate etc.

The code above assumes that the Allocator is the std::pmr::polymorphic_allocator which is correct (for this case) but it doesn't take into account the memory_resource. That being said, the following assertions trigger:

class MyMemoryResource : public std::memory_resource {
// some logic here
};

auto *my_memory_resource = new MyMemoryResource()
jsoncons::pmr::json js(std::polymorphic_allocator(my_memory_resource));
assert(my_memory_resource == json.get_allocator().resource()); // Triggers

The same applies for copy constructors:
jsoncons::pmr::json js(some_other_json, std::polymorphic_allocator(my_memory_resource));
assert(my_memory_resource == json.get_allocator().resource()); // passes

Which is different from the standard behavior (you can verify this by replacing jsoncons::pmr::json with std::vector and the assertion should pass.

I am happy to contribute and take care of those two cases but I think it would be nice if we can somehow track all of those and fix them.

This is something that we need to properly track memory usage in our system.

Best regards.

[danielaparker]

Thanks for reporting. I'm off on vacation today, and won't be back until I return on Sep 24, but will investigate then.

[kostasrim]

Thank you for replying @danielaparker! Have a good one and we talk when you are back ❤️

[kostasrim]

I also noticed that copy assignment does not poll std::allocator_traits<T>::propagate_on_container_copy_assignment (same for move_assignment) which is also problematic. For copy/move ( basic_json(const basic_json& other)) we need to poll (select_on_container_copy_construction).

[danielaparker]

@kostasrim, to address your points:

• Case A did ignore the allocator. This constructor wasn't officially supported (not documented), but did create a basic_json of empty object storage (same as basic_json()). I've changed it to be consistent with basic_json(json_object_arg_t, const Allocator& alloc).

• Case B did not ignore the allocator (not sure how you came to that conclusion).

• Copy constructors for long strings, byte strings, arrays and objects (the basic_json types having allocators) did not use std::allocator_traits<allocator_type>::select_on_container_copy_construction. Fixing this changed behaviour because std::pmr::polymorphic_allocator<T>::select_on_container_copy_construction returns a default-constructed polymorphic_allocator object, not the argument.

• basic_json copy assignment does not use std::allocator_traits<T>::propagate_on_container_copy_assignment directly, however, it does now use it indirectly for arrays and objects due to them being implemented with std::vector. For long strings, copy assignment to another long string (or array or object) always deallocates the memory of the target element and reallocates using the allocator of the target element, that is, the pessimistic and for long strings the only strategy. Previously, array and object assignment also always used a pessimistic strategy, but I've improved that while addressing the issues you raised.

• basic_json move assignment does not use std::allocator_traits<T>::propagate_on_container_move_assignment at all. Note that basic_json is not a container, an allocator is not stored with a basic_json itself, rather, a basic_json is variant like and may contain different kinds of elements, some of which have allocators (a long string, byte string, array, or object), and some of which do not (a number, boolean, or null). An element such as an array that requires an allocator is stored as a pointer to that element, and the allocator required to deallocate it is stored with the element. A move assignment from a basic_json array to a basic_json object, for example, is just a swap of two pointers.

I've added a sizeable number of test cases for copy construction and move construction with polymorphic_allocator in json_constructor_tests.cpp, and for copy and move assignment in json_assignment_tests.cpp

Below are a few of the cases covered in the tests. Please feel free to propose additional tests, or ask for justification for expected results.

#include <jsoncons/json.hpp>
#include <memory_resource> 
#include <cassert>

int main() 
{
    using allocator_type = std::pmr::polymorphic_allocator<char>;

    char buffer1[1024] = {}; // a small buffer on the stack
    char* last1 = buffer1 + sizeof(buffer1);
    std::pmr::monotonic_buffer_resource pool1{ std::data(buffer1), std::size(buffer1) };
    allocator_type alloc1(&pool1);

    char buffer2[1024] = {}; // another small buffer on the stack
    char* last2 = buffer2 + sizeof(buffer2);
    std::pmr::monotonic_buffer_resource pool2{ std::data(buffer2), std::size(buffer2) };
    allocator_type alloc2(&pool2);

    const char* long_key = "Key too long for short string";
    const char* long_key_end = long_key + strlen(long_key);

    const char* long_string = "String too long for short string";
    const char* long_string_end = long_string + strlen(long_string);

    const char* another_long_string = "Another string too long for short string";
    const char* another_long_string_end = another_long_string + strlen(another_long_string);

    // Construct a jsoncons::pmr::json with an allocator
    jsoncons::pmr::json j1{alloc1};
    assert(j1.is_object());
    assert(&pool1 == j1.get_allocator().resource());

    // Copy construct a jsoncons::pmr::json with a long string
    jsoncons::pmr::json j2{ long_string, alloc1 };
    assert(j2.is_string());
    assert(&pool1 == j2.get_allocator().resource());

    // Copy construct a jsoncons::pmr::json with a json long string with alloc1
    jsoncons::pmr::json j3(j2);
    assert(j3.is_string());
    assert(j3.get_allocator() == std::allocator_traits<allocator_type>::select_on_container_copy_construction(j2.get_allocator()));
    assert(j3.get_allocator() == allocator_type{});

    // Move construct a jsoncons::pmr::json with a json long string with alloc1
    jsoncons::pmr::json j4(std::move(j2));
    assert(j4.is_string());
    assert(&pool1 == j4.get_allocator().resource());

    // Copy a json array having alloc2 to a json array having alloc1
    jsoncons::pmr::json j5{ jsoncons::json_array_arg, alloc1 };
    assert(&pool1 == j5.get_allocator().resource());
    j5.push_back(long_string);
    auto it = std::search(buffer1, last1, long_string, long_string_end);
    assert(it != last1);

    jsoncons::pmr::json j6{ jsoncons::json_array_arg, alloc2 };
    assert(&pool2 == j6.get_allocator().resource());
    j6.push_back(another_long_string);
    it = std::search(buffer2, last2, another_long_string, another_long_string_end);
    assert(it != last2);

    j5 = j6;
    assert(&pool1 == j5.get_allocator().resource());
    it = std::search(buffer1, last1, another_long_string, another_long_string_end);
    assert(j5 == j6);

    // Copy a json object having alloc2 to a json number having no allocator 
    jsoncons::pmr::json j7{ 10 };
    assert(j7.is_number());

    jsoncons::pmr::json j8{ alloc2 };
    assert(j8.is_object());
    j8.insert_or_assign(long_key, long_string);

    j7 = j8;
    assert(j7.get_allocator() == std::allocator_traits<allocator_type>::select_on_container_copy_construction(j8.get_allocator()));
    assert(j7.get_allocator() == allocator_type{});

    // Move a json object having alloc2 to a json number having no allocator  
    jsoncons::pmr::json j9{ 10 };
    assert(j9.is_number());

    j9 = std::move(j8);
    assert(&pool2 == j9.get_allocator().resource());
    it = std::search(buffer2, last2, long_key, long_key_end);
    assert(it != last2);
    it = std::search(buffer2, last2, long_string, long_string_end);
    assert(it != last2);
}