[libcxx-commits] [PATCH] D89057: Add the C++17 <memory_resource> header (mono-patch)
Nikolas Klauser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jan 6 17:12:11 PST 2022
philnik requested changes to this revision.
philnik added a reviewer: philnik.
philnik added a comment.
I didn't get all the way through, but here are about 20 nit picks. I'll look at the tests later.
================
Comment at: libcxx/include/__memory_resource/memory_resource.h:29
+{
+ static const size_t __max_align = alignof(max_align_t);
+
----------------
Is there a reason this isn't constexpr or just directly in lines 35 and 39?
================
Comment at: libcxx/include/__memory_resource/memory_resource.h:36
+ void* allocate(size_t __bytes, size_t __align = __max_align)
+ { return do_allocate(__bytes, __align); }
+
----------------
I hate this brace style with all my heart, but it's used through out the code base so I guess I can't do anything about it.
================
Comment at: libcxx/include/__memory_resource/memory_resource.h:39
+ _LIBCPP_HIDE_FROM_ABI
+ void deallocate(void *__p, size_t __bytes, size_t __align = __max_align)
+ { do_deallocate(__p, __bytes, __align); }
----------------
Nit: Use `void*` instead of `void *` or at least do it consistently (see line 35).
================
Comment at: libcxx/include/__memory_resource/memory_resource.h:68-78
+_LIBCPP_FUNC_VIS
+memory_resource *get_default_resource() noexcept;
+
+_LIBCPP_FUNC_VIS
+memory_resource *set_default_resource(memory_resource *) noexcept;
+
+_LIBCPP_FUNC_VIS
----------------
================
Comment at: libcxx/include/__memory_resource/synchronized_pool_resource.h:61
+#if !defined(_LIBCPP_HAS_NO_THREADS)
+ unique_lock<mutex> __lk(__mut_);
+#endif
----------------
Remember https://youtu.be/F6Ipn7gCOsY?t=1835?
================
Comment at: libcxx/include/__memory_resource/unsynchronized_pool_resource.h:49-57
+ static const size_t __min_blocks_per_chunk = 16;
+ static const size_t __min_bytes_per_chunk = 1024;
+ static const size_t __max_blocks_per_chunk = (size_t(1) << 20);
+ static const size_t __max_bytes_per_chunk = (size_t(1) << 30);
+
+ static const int __log2_smallest_block_size = 3;
+ static const size_t __smallest_block_size = 8;
----------------
Again - is there a reason these aren't constexpr?
================
Comment at: libcxx/include/__memory_resource/unsynchronized_pool_resource.h:96
+protected:
+ void *do_allocate(size_t __bytes, size_t __align) override; // key function
+
----------------
What do you want to say with `// key function`? For the vtable?
================
Comment at: libcxx/include/deque:3027-3035
+#if _LIBCPP_STD_VER > 14
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace pmr
+{
+ template <class _ValueT>
+ using deque = _VSTD::deque<_ValueT, polymorphic_allocator<_ValueT>>;
+}
----------------
Why is this not just in the above std namespace?
================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:26-60
+ Symbol added: __ZNKSt3__13pmr26synchronized_pool_resource11do_is_equalERKNS0_15memory_resourceE
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource12__pool_indexEmm
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource17__pool_block_sizeEi
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource22__log2_pool_block_sizeEi
+ Symbol added: __ZNKSt3__13pmr28unsynchronized_pool_resource7optionsEv
+ Symbol added: __ZNSt3__13pmr19new_delete_resourceEv
+ Symbol added: __ZNSt3__13pmr20get_default_resourceEv
----------------
Are these symbols not added under linux?
================
Comment at: libcxx/src/memory_resource.cpp:17
+#if defined(__ELF__) && defined(_LIBCPP_LINK_PTHREAD_LIB)
+#pragma comment(lib, "pthread")
+#endif
----------------
Doesn't this only work with lld?
================
Comment at: libcxx/src/memory_resource.cpp:116-117
+ // TODO: Can a weaker ordering be used?
+ return _VSTD::atomic_exchange_explicit(
+ &__res, new_res, memory_order_acq_rel);
+ }
----------------
================
Comment at: libcxx/src/memory_resource.cpp:126-135
+ if (set) {
+ new_res = new_res ? new_res : new_delete_resource();
+ lock_guard<mutex> guard(res_lock);
+ memory_resource *old_res = res;
+ res = new_res;
+ return old_res;
+ } else {
----------------
And with the other `set` checks as well.
================
Comment at: libcxx/src/memory_resource.cpp:127
+ if (set) {
+ new_res = new_res ? new_res : new_delete_resource();
+ lock_guard<mutex> guard(res_lock);
----------------
================
Comment at: libcxx/src/memory_resource.cpp:176-177
+
+void unsynchronized_pool_resource::__adhoc_pool::__release_ptr(
+ memory_resource *upstream)
+{
----------------
================
Comment at: libcxx/src/memory_resource.cpp:190-191
+{
+ const size_t footer_size = sizeof(__chunk_footer);
+ const size_t footer_align = alignof(__chunk_footer);
+
----------------
================
Comment at: libcxx/src/memory_resource.cpp:212
+{
+ _LIBCPP_ASSERT(__first_ != nullptr, "deallocating a block that was not allocated with this allocator");
+ if (__first_->__start_ == p) {
----------------
Wouldn't it make more sense to put that in the header so the dylib doesn't have to change depending on wanting these assertions or not?
================
Comment at: libcxx/src/memory_resource.cpp:213-228
+ if (__first_->__start_ == p) {
+ __chunk_footer *next = __first_->__next_;
+ upstream->deallocate(p, __first_->__allocation_size(), __first_->__align_);
+ __first_ = next;
+ } else {
+ for (__chunk_footer *h = __first_; h->__next_ != nullptr; h = h->__next_) {
+ if (h->__next_->__start_ == p) {
----------------
================
Comment at: libcxx/src/memory_resource.cpp:335-345
+ else {
+ int i = 0;
+ bytes = (bytes > align) ? bytes : align;
+ bytes -= 1;
+ bytes >>= __log2_smallest_block_size;
+ while (bytes != 0) {
+ bytes >>= 1;
----------------
================
Comment at: libcxx/src/memory_resource.cpp:413
+ return __adhoc_pool_.__do_allocate(__res_, bytes, align);
+ else {
+ if (__fixed_pools_ == nullptr) {
----------------
Please remove this else as well.
================
Comment at: libcxx/src/memory_resource.cpp:426-427
+ if (result == nullptr) {
+ auto min = [](size_t a, size_t b) { return a < b ? a : b; };
+ auto max = [](size_t a, size_t b) { return a < b ? b : a; };
+
----------------
Why not use std::min and std::max?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89057/new/
https://reviews.llvm.org/D89057
More information about the libcxx-commits
mailing list