You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by bgaff <gi...@git.apache.org> on 2016/05/09 16:08:20 UTC
[GitHub] trafficserver pull request: Standard library allocators and API to...
GitHub user bgaff opened a pull request:
https://github.com/apache/trafficserver/pull/622
Standard library allocators and API to expose IOBuf memory allocation.
This is a continuation of the pull request #556 (Adding STL Io Buf Allocators). This pull request contains that same code and it also adds three new memory related APIs for plugins, TSmalloc_pool, TSrealloc_pool, and TSfree_pool. It also modifies a plugin to show the usage.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bgaff/trafficserver memory_allocation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/622.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #622
----
commit 9df29ec235e88aadad654c01a9fe667b15dc6ed4
Author: Brian Geffon <br...@apache.org>
Date: 2016-05-09T01:20:35Z
Standard library allocators and API to expose IOBuf memory allocation.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-218872975
@bgaff You want to just close this out for now, until we all test / verify the validity of turning off freelist in favor of just jemalloc ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62567399
--- Diff: lib/ts/ink_memory.h ---
@@ -89,6 +89,11 @@ void *ats_free_null(void *ptr);
void ats_memalign_free(void *ptr);
int ats_mallopt(int param, int value);
+/* use IOBufs as the underlying allocation pool */
+void *ats_malloc_pool(size_t size);
+void *ats_realloc_pool(void *ptr, size_t size);
+void ats_free_pool(void *ptr);
--- End diff --
Yeah, James is right, definitely not awesome to have cross pollination of implementations and interfaces across module boundaries :-/. Probably should go into I_IOBuffer.h ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by dragon512 <gi...@git.apache.org>.
Github user dragon512 commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-217930908
Given what I can see everything looks correct in the allocate code. The only concern I have is that the rebind function can be inherited correctly as implemented. Cases like list, map always call a rebind-ed allocator to get make a node object that holds the _Type and bound data.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #622: TS-2581: Standard library allocators and API to ex...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the issue:
https://github.com/apache/trafficserver/pull/622
Did we ever finalize the jemalloc discussion? I know I saw improved performance with --disable-freelist and LD_PRELOADing jemalloc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff closed the pull request at:
https://github.com/apache/trafficserver/pull/622
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62564614
--- Diff: iocore/eventsystem/IOBuffer.cc ---
@@ -271,3 +272,74 @@ IOBufferReader::memcpy(const void *ap, int64_t len, int64_t offset)
return p;
}
+
+void *
+ats_malloc_pool(size_t size)
+{
+ void *ptr = NULL;
+ size_t total_required_size = size + sizeof(uint64_t);
+
+ if (likely(size > 0)) {
+ if (unlikely(total_required_size > DEFAULT_MAX_BUFFER_SIZE)) {
+ // we need to fall back to ats_malloc for large allocations
+ ptr = ats_malloc(total_required_size);
+ } else {
+ int64_t iobuffer_index = iobuffer_size_to_index(total_required_size);
+ ink_release_assert(iobuffer_index >= 0);
+
+ ptr = ioBufAllocator[iobuffer_index].alloc_void();
+ Debug("allocator_pool",
+ "Allocating a block from iobuf index %ld (block size %ld), requested_size = %ld, alloc size = %ld at location %p",
+ iobuffer_index, index_to_buffer_size(iobuffer_index), size, total_required_size, ptr);
+ }
+ } else {
+ return NULL;
+ }
--- End diff --
In ``ats_realloc_pool`` you do an early return on NULL, I think you should do the same here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62565638
--- Diff: iocore/eventsystem/IOBuffer.cc ---
@@ -271,3 +272,74 @@ IOBufferReader::memcpy(const void *ap, int64_t len, int64_t offset)
return p;
}
+
+void *
+ats_malloc_pool(size_t size)
+{
+ void *ptr = NULL;
+ size_t total_required_size = size + sizeof(uint64_t);
+
+ if (likely(size > 0)) {
+ if (unlikely(total_required_size > DEFAULT_MAX_BUFFER_SIZE)) {
+ // we need to fall back to ats_malloc for large allocations
+ ptr = ats_malloc(total_required_size);
+ } else {
+ int64_t iobuffer_index = iobuffer_size_to_index(total_required_size);
+ ink_release_assert(iobuffer_index >= 0);
+
+ ptr = ioBufAllocator[iobuffer_index].alloc_void();
+ Debug("allocator_pool",
+ "Allocating a block from iobuf index %ld (block size %ld), requested_size = %ld, alloc size = %ld at location %p",
+ iobuffer_index, index_to_buffer_size(iobuffer_index), size, total_required_size, ptr);
+ }
--- End diff --
This looks really similar to ``IOBufferData::alloc``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62565914
--- Diff: lib/ts/ink_memory.h ---
@@ -89,6 +89,11 @@ void *ats_free_null(void *ptr);
void ats_memalign_free(void *ptr);
int ats_mallopt(int param, int value);
+/* use IOBufs as the underlying allocation pool */
+void *ats_malloc_pool(size_t size);
+void *ats_realloc_pool(void *ptr, size_t size);
+void ats_free_pool(void *ptr);
--- End diff --
``ats_malloc`` lives in libtsutil.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62565475
--- Diff: iocore/eventsystem/IOBuffer.cc ---
@@ -271,3 +272,74 @@ IOBufferReader::memcpy(const void *ap, int64_t len, int64_t offset)
return p;
}
+
+void *
+ats_malloc_pool(size_t size)
--- End diff --
Since this is in iocore, you should follow the naming conventions of iocore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62566642
--- Diff: lib/ts/ink_memory.h ---
@@ -89,6 +89,11 @@ void *ats_free_null(void *ptr);
void ats_memalign_free(void *ptr);
int ats_mallopt(int param, int value);
+/* use IOBufs as the underlying allocation pool */
+void *ats_malloc_pool(size_t size);
+void *ats_realloc_pool(void *ptr, size_t size);
+void ats_free_pool(void *ptr);
--- End diff --
It is fine for it to be implemented in iocore. But once you do that you can't put the declarations at some other layer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Standard library allocators and API to...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-217910557
cc @jpeach @PSUdaemon @zwoop @SolidWallOfCode @bryancall @sudheerv @dragon512
Let's use this PR to finalize the naming conventions we want for this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62565771
--- Diff: lib/ts/ink_memory.h ---
@@ -89,6 +89,11 @@ void *ats_free_null(void *ptr);
void ats_memalign_free(void *ptr);
int ats_mallopt(int param, int value);
+/* use IOBufs as the underlying allocation pool */
+void *ats_malloc_pool(size_t size);
+void *ats_realloc_pool(void *ptr, size_t size);
+void ats_free_pool(void *ptr);
--- End diff --
All the other memory declarations (like ats_malloc) are already in ink_memory.h, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62564491
--- Diff: lib/ts/ink_memory.h ---
@@ -89,6 +89,11 @@ void *ats_free_null(void *ptr);
void ats_memalign_free(void *ptr);
int ats_mallopt(int param, int value);
+/* use IOBufs as the underlying allocation pool */
+void *ats_malloc_pool(size_t size);
+void *ats_realloc_pool(void *ptr, size_t size);
+void ats_free_pool(void *ptr);
--- End diff --
Since the implementation of these functions is in iocore, the declarations should be too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-217979176
(TS-2851)[https://issues.apache.org/jira/browse/TS-2581] is asking to be explicit about allocation from IO buffers, but this is doing implicit allocator switches like the existing code does. Maybe we can resolve which approach we prefer?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-219259861
Yes @zwoop let's close this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Standard library allocators and API to...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-217912734
Using this same patter we can easily extend the new APIs to provide Standard Library Allocators for C++ APIs or plugins that use the C++ standard library.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by dragon512 <gi...@git.apache.org>.
Github user dragon512 commented on the pull request:
https://github.com/apache/trafficserver/pull/622#issuecomment-217931548
no wait.. I missed it the code it is there . This looks fine
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62565756
--- Diff: iocore/eventsystem/IOBuffer.cc ---
@@ -271,3 +272,74 @@ IOBufferReader::memcpy(const void *ap, int64_t len, int64_t offset)
return p;
}
+
+void *
+ats_malloc_pool(size_t size)
+{
+ void *ptr = NULL;
+ size_t total_required_size = size + sizeof(uint64_t);
+
+ if (likely(size > 0)) {
+ if (unlikely(total_required_size > DEFAULT_MAX_BUFFER_SIZE)) {
+ // we need to fall back to ats_malloc for large allocations
+ ptr = ats_malloc(total_required_size);
+ } else {
+ int64_t iobuffer_index = iobuffer_size_to_index(total_required_size);
+ ink_release_assert(iobuffer_index >= 0);
+
+ ptr = ioBufAllocator[iobuffer_index].alloc_void();
+ Debug("allocator_pool",
+ "Allocating a block from iobuf index %ld (block size %ld), requested_size = %ld, alloc size = %ld at location %p",
+ iobuffer_index, index_to_buffer_size(iobuffer_index), size, total_required_size, ptr);
+ }
+ } else {
+ return NULL;
+ }
--- End diff --
Well I disagree that consistency is bike shedding :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62565535
--- Diff: iocore/eventsystem/IOBuffer.cc ---
@@ -271,3 +272,74 @@ IOBufferReader::memcpy(const void *ap, int64_t len, int64_t offset)
return p;
}
+
+void *
+ats_malloc_pool(size_t size)
+{
+ void *ptr = NULL;
+ size_t total_required_size = size + sizeof(uint64_t);
+
+ if (likely(size > 0)) {
+ if (unlikely(total_required_size > DEFAULT_MAX_BUFFER_SIZE)) {
+ // we need to fall back to ats_malloc for large allocations
+ ptr = ats_malloc(total_required_size);
+ } else {
+ int64_t iobuffer_index = iobuffer_size_to_index(total_required_size);
+ ink_release_assert(iobuffer_index >= 0);
+
+ ptr = ioBufAllocator[iobuffer_index].alloc_void();
+ Debug("allocator_pool",
+ "Allocating a block from iobuf index %ld (block size %ld), requested_size = %ld, alloc size = %ld at location %p",
+ iobuffer_index, index_to_buffer_size(iobuffer_index), size, total_required_size, ptr);
+ }
+ } else {
+ return NULL;
+ }
--- End diff --
Sounds like bike shedding, it's a style preference.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-2581: Standard library allocators a...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/622#discussion_r62566454
--- Diff: lib/ts/ink_memory.h ---
@@ -89,6 +89,11 @@ void *ats_free_null(void *ptr);
void ats_memalign_free(void *ptr);
int ats_mallopt(int param, int value);
+/* use IOBufs as the underlying allocation pool */
+void *ats_malloc_pool(size_t size);
+void *ats_realloc_pool(void *ptr, size_t size);
+void ats_free_pool(void *ptr);
--- End diff --
That's why it landed in iocore it would have been a pain to deal with dependencies between libtsutil and iocore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---