You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/12/07 22:01:43 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
ywkaras opened a new pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241
Replaces https://github.com/apache/trafficserver/pull/6153 .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-740788947
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-738635293
I'm buying this idea. Especially, we can get rid of a hack of `force_VFPT_to_top`.
I'd like to get more eyeballs, because this is a fundamental change of Allocator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769863306
The asan build crashed too. One stack the same as the earlier two. The other stack different but also due to uninitialized od member variable.
```
(gdb) bt
#0 0x0000000000ccd8cf in CacheVC::openWriteStartDone(int, Event*) () at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheWrite.cc:1579
#1 0x0000000000faa6b9 in handleEvent (data=0x609000100c10, event=2, this=<optimized out>) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#2 handleEvent (data=0x609000100c10, event=2, this=<optimized out>) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#3 EThread::process_event(Event*, int) () at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:132
#4 0x0000000000facc9c in EThread::execute_regular (this=this@entry=0x2b28e3c11800) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:241
#5 0x0000000000faddee in execute (this=0x2b28e3c11800) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:332
#6 EThread::execute (this=0x2b28e3c11800) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:310
#7 0x0000000000fa812b in spawn_thread_internal (a=0x6060000a7fc0) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/Thread.cc:92
#8 0x00002b28d975eea5 in start_thread (arg=0x2b28ea573700) at pthread_create.c:307
#9 0x00002b28da69c8dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(gdb) print od
$1 = (OpenDirEntry *) 0xbebebebebebebebe
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769463079
I can benchmark this and see if there is any differences in performance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769863306
The asan build crashed too. One stack the same as the earlier two. The other stack different but also due to uninitialized ok member variable.
```
(gdb) bt
#0 0x0000000000ccd8cf in CacheVC::openWriteStartDone(int, Event*) () at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheWrite.cc:1579
#1 0x0000000000faa6b9 in handleEvent (data=0x609000100c10, event=2, this=<optimized out>) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#2 handleEvent (data=0x609000100c10, event=2, this=<optimized out>) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#3 EThread::process_event(Event*, int) () at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:132
#4 0x0000000000facc9c in EThread::execute_regular (this=this@entry=0x2b28e3c11800) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:241
#5 0x0000000000faddee in execute (this=0x2b28e3c11800) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:332
#6 EThread::execute (this=0x2b28e3c11800) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:310
#7 0x0000000000fa812b in spawn_thread_internal (a=0x6060000a7fc0) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/Thread.cc:92
#8 0x00002b28d975eea5 in start_thread (arg=0x2b28ea573700) at pthread_create.c:307
#9 0x00002b28da69c8dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(gdb) print od
$1 = (OpenDirEntry *) 0xbebebebebebebebe
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-664585219
[approve ci FreeBSD]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r537619263
##########
File path: include/tscore/Allocator.h
##########
@@ -152,7 +153,7 @@ template <class C> class ClassAllocator : public Allocator
@param tail pointer to be freed.
@param num_item of blocks to be freed.
*/
- void
+ std::enable_if_t<!Destruct_on_free, void>
Review comment:
Sorry, I did this wrong, should have tested it better. I'll fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769870712
initializing od to nullptr in the definition of CacheVC (P_CacheInternal.h), seems to have solved the start up crash. I also verified that the start up did not blow the cache. After start up, the canary machine has about the same cache hit rate as its unchanged peer. I'm going back to a non-ASAN build, and I'll let that machine run for another day or two.
@SolidWallOfCode pointed out that the constructor 0's "part" of the new object. Is it possible that sizeof(CacheVC) changes with this PR? I don't see how, but perhaps I'm missing something. Otherwise, I don't see how this initialization logic differs with and without this patch.
```
CacheVC::CacheVC()
{
size_to_init = sizeof(CacheVC) - (size_t) & ((CacheVC *)nullptr)->vio;
memset((void *)&vio, 0, size_to_init);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r535869238
##########
File path: include/tscore/Allocator.h
##########
@@ -142,6 +140,9 @@ template <class C> class ClassAllocator : public Allocator
void
free(C *ptr)
{
+ if (Destruct_on_free) {
Review comment:
Seems a nice idea. I always struggle with how to destroy/free objects when I use `ClassAllocator`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r535909429
##########
File path: include/tscore/Allocator.h
##########
@@ -152,7 +153,7 @@ template <class C> class ClassAllocator : public Allocator
@param tail pointer to be freed.
@param num_item of blocks to be freed.
*/
- void
+ std::enable_if_t<!Destruct_on_free, void>
Review comment:
Is this a guard to do not set `Destruct_on_free` true?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-771132376
> My test build ran all weekend. The numbers (rps and average latency) look slightly better than its peer. Once the change is made in the CacheVC constructor to initialize the od member to nullptr, I'd endorse this change to be merged.
Should this change be made as a part of this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-740788947
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-786266356
Hey @masaori335 can you take another look at this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-842507102
Cherry-picked to v9.1.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 edited a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 edited a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-738635293
I'm buying this idea. Especially, we can get rid of the hack of `force_VFPT_to_top`.
I'd like to get more eyeballs, because this is a fundamental change of Allocator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r567014393
##########
File path: include/tscore/Allocator.h
##########
@@ -108,29 +109,39 @@ class Allocator
ink_freelist_madvise_init(&this->fl, name, element_size, chunk_size, alignment, advice);
}
+ // Dummies
+ void
+ destroy_if_enabled(void *)
+ {
+ }
+ Allocator &
+ raw()
+ {
+ return *this;
+ }
+
protected:
InkFreeList *fl;
};
/**
- Allocator for Class objects. It uses a prototype object to do
- fast initialization. Prototype of the template class is created
- when the fast allocator is created. This is instantiated with
- default (no argument) constructor. Constructor is not called for
- the allocated objects. Instead, the prototype is just memory
- copied onto the new objects. This is done for performance reasons.
+ Allocator for Class objects.
*/
-template <class C> class ClassAllocator : public Allocator
+template <class C, bool Destruct_on_free_ = false> class ClassAllocator : private Allocator
{
public:
- /** Allocates objects of the templated type. */
+ using Value_type = C;
+ static bool const Destruct_on_free = Destruct_on_free_;
+
+ /** Allocates objects of the templated type. Arguments are forwarded to the constructor for the object. */
+ template <typename... Args>
C *
- alloc()
+ alloc(Args &&... args)
{
void *ptr = ink_freelist_new(this->fl);
- memcpy(ptr, (void *)&this->proto.typeObject, sizeof(C));
+ ::new (ptr) C(std::forward<Args>(args)...);
Review comment:
"proto" is a data member of the class allocator, and I think they are always statically allocated. So the raw memory for proto will be zeroed out at load time before the proto object is constructed by placement new. We could zero the memory here before calling placement new. If we don't, there is a risk of seeing further crashes with this change, like the one Susan found.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769847408
I applied this patch to our current production ATS9 build, and it core's on start up with the following stack. Appears that something is not getting correctly initialized on reading in the cache. The member variable od of type OpenDirEntry appears to be bogus value. I'm setting up an ASAN build to hopefully get more details.
```
(gdb) bt
#0 0x00000000006cdaae in remove (this=0xf8f4bb5c, e=0x2b6f1d20d800) at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheDir.cc:138
#1 OpenDir::close_write (this=0x2b6cea1061e0, cont=cont@entry=0x2b6f1d20d800) at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheDir.cc:138
#2 0x00000000006ede4c in close_write (cont=0x2b6f1d20d800, this=<optimized out>) at ../../../../../../_vcs/trafficserver9/iocore/cache/P_CacheInternal.h:765
#3 CacheVC::openWriteCloseDir(int, Event*) () at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheWrite.cc:1112
#4 0x00000000006f1e86 in Cache::open_write(Continuation*, ats::CryptoHash const*, HTTPInfo*, long, ats::CryptoHash const*, CacheFragType, char const*, int) ()
at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheWrite.cc:1831
#5 0x00000000006bc925 in CacheProcessor::open_write (this=<optimized out>, cont=cont@entry=0x2b6ceed085c8, expected_size=expected_size@entry=0, key=key@entry=0x2b6ce440d610,
request=request@entry=0x2b6ceed06fb0, old_info=<optimized out>, pin_in_cache=0, type=CACHE_FRAG_TYPE_HTTP) at ../../../../../../_vcs/trafficserver9/iocore/cache/Cache.cc:3254
#6 0x00000000005dd2a8 in HttpCacheSM::open_write (this=this@entry=0x2b6ceed085c8, key=key@entry=0x2b6ce440d610, url=url@entry=0x2b6ceed06938, request=request@entry=0x2b6ceed06fb0,
old_info=old_info@entry=0x2b6f1d206558, pin_in_cache=<optimized out>, retry=true, allow_multiple=false) at ../../../../../../_vcs/trafficserver9/proxy/http/HttpCacheSM.cc:362
#7 0x0000000000564a23 in HttpSM::do_cache_prepare_action(HttpCacheSM*, HTTPInfo*, bool, bool) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:4722
#8 0x0000000000582765 in do_cache_prepare_write (this=0x2b6ceed06800) at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:4652
#9 HttpSM::set_next_state() () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:7596
#10 0x00000000005756b1 in HttpSM::state_api_callout(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1547
#11 0x0000000000579c07 in HttpSM::state_api_callback(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1351
#12 0x000000000051361f in TSHttpTxnReenable () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:6164
#13 0x00002b6cef802a5d in (anonymous namespace)::cb (contp=<optimized out>, event=<optimized out>, edata=0x2b6ceed06800) at _vcs/quick_filter-9/quick_filter/quick_filter.cc:637
#14 0x00000000004fe1d0 in INKContInternal::handle_event(int, void*) () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:1096
#15 0x0000000000510a9b in handleEvent (data=0x2b6ceed06800, event=60003, this=0x2b6cd6650a00)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#16 handleEvent (data=0x2b6ceed06800, event=60003, this=0x2b6cd6650a00)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#17 APIHook::invoke(int, void*) const () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:1333
#18 0x00000000005754ac in HttpSM::state_api_callout(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1475
#19 0x0000000000582abb in HttpSM::set_next_state() () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:7355
#20 0x000000000056ab42 in HttpSM::do_hostdb_lookup() () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:4175
#21 0x0000000000582b5a in HttpSM::set_next_state() () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:7671
#22 0x00000000005756b1 in HttpSM::state_api_callout(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1547
#23 0x0000000000579c07 in HttpSM::state_api_callback(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1351
#24 0x000000000051361f in TSHttpTxnReenable () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:6164
#25 0x00002b6d07cda97e in main_handler (cont=<optimized out>, event=<optimized out>, edata=0x2b6ceed06800) at ../../../../../_vcs/trafficserver9/plugins/regex_revalidate/regex_revalidate.c:453
#26 0x00000000004fe1d0 in INKContInternal::handle_event(int, void*) () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:1096
#27 0x0000000000510a9b in handleEvent (data=0x2b6ceed06800, event=60015, this=0x2b6cda99b980)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#28 handleEvent (data=0x2b6ceed06800, event=60015, this=0x2b6cda99b980)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#29 APIHook::invoke(int, void*) const () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:1333
#30 0x00000000005754ac in HttpSM::state_api_callout(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1475
#31 0x0000000000579c07 in HttpSM::state_api_callback(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1351
#32 0x000000000051361f in TSHttpTxnReenable () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:6164
#33 0x00002b6cfab731cd in carpLookup(tsapi_cont*, TSEvent, void*) () at _vcs/carp-9/carp/carp.cc:767
#34 0x00000000004fe1d0 in INKContInternal::handle_event(int, void*) () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:1096
#35 0x0000000000510a9b in handleEvent (data=0x2b6ceed06800, event=60015, this=0x2b6cd6652120)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#36 handleEvent (data=0x2b6ceed06800, event=60015, this=0x2b6cd6652120)
---Type <return> to continue, or q <return> to quit---
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#37 APIHook::invoke(int, void*) const () at ../../../../../_vcs/trafficserver9/src/traffic_server/InkAPI.cc:1333
#38 0x00000000005754ac in HttpSM::state_api_callout(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1475
#39 0x0000000000582abb in HttpSM::set_next_state() () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:7355
#40 0x00000000005756b1 in HttpSM::state_api_callout(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:1547
#41 0x0000000000582abb in HttpSM::set_next_state() () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:7355
#42 0x0000000000577bce in HttpSM::state_cache_open_read(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:2553
#43 0x0000000000578a7b in HttpSM::main_handler(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpSM.cc:2615
#44 0x00000000005dd031 in handleEvent (data=0x2b6f1d206400, event=1102, this=0x2b6ceed06800)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#45 handleEvent (data=0x2b6f1d206400, event=1102, this=0x2b6ceed06800)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#46 HttpCacheSM::state_cache_open_read(int, void*) () at ../../../../../../_vcs/trafficserver9/proxy/http/HttpCacheSM.cc:114
#47 0x00000000006e7717 in handleEvent (data=0x2b6f1d206400, event=1102, this=0x2b6ceed085c8)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#48 handleEvent (data=0x2b6f1d206400, event=1102, this=0x2b6ceed085c8)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#49 CacheVC::callcont (this=this@entry=0x2b6f1d206400, event=event@entry=1102) at ../../../../../../_vcs/trafficserver9/iocore/cache/P_CacheInternal.h:639
#50 0x00000000006e2f8e in CacheVC::openReadStartEarliest(int, Event*) () at ../../../../../../_vcs/trafficserver9/iocore/cache/CacheRead.cc:996
#51 0x00000000006c41f4 in CacheVC::handleReadDone(int, Event*) () at ../../../../../../_vcs/trafficserver9/iocore/cache/Cache.cc:2301
#52 0x00000000006c6b1e in AIOCallbackInternal::io_complete (this=0x2b6f1d206588, event=<optimized out>, data=<optimized out>)
at /home/shinrich/build-walt/_build/build_release_posix-x86_64_gcc_8/trafficserver9/build/../../../../_vcs/trafficserver9/iocore/aio/P_AIO.h:121
#53 0x00000000007cf2fb in handleEvent (data=0x2b6d25601360, event=1, this=0x2b6f1d206588) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:167
#54 handleEvent (data=0x2b6d25601360, event=1, this=0x2b6f1d206588) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/I_Continuation.h:163
#55 EThread::process_event(Event*, int) () at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:132
#56 0x00000000007cf9ce in EThread::process_queue (this=this@entry=0x2b6cda63a740, NegativeQueue=NegativeQueue@entry=0x2b6ce440e600, ev_count=ev_count@entry=0x2b6ce440e5fc,
nq_count=nq_count@entry=0x2b6ce440e5f8) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:167
#57 0x00000000007cfe34 in EThread::execute_regular (this=this@entry=0x2b6cda63a740) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:227
#58 0x00000000007d04c6 in execute (this=0x2b6cda63a740) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:332
#59 EThread::execute (this=0x2b6cda63a740) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/UnixEThread.cc:310
#60 0x00000000007ce8a9 in spawn_thread_internal (a=0x2b6cd2adf840) at ../../../../../../_vcs/trafficserver9/iocore/eventsystem/Thread.cc:92
#61 0x00002b6cd0df8ea5 in start_thread (arg=0x2b6ce4410700) at pthread_create.c:307
#62 0x00002b6cd1b2e8dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(gdb)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769870712
initializing od to nullptr in the definition of CacheVC (P_CacheInternal.h), seems to have solved the start up crash.
@SolidWallOfCode pointed out that the constructor 0's "part" of the new object. Is it possible that sizeof(CacheVC) changes with this PR? I don't see how, but perhaps I'm missing something. Otherwise, I don't see how this initialization logic differs with and without this patch.
```
CacheVC::CacheVC()
{
size_to_init = sizeof(CacheVC) - (size_t) & ((CacheVC *)nullptr)->vio;
memset((void *)&vio, 0, size_to_init);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r537243211
##########
File path: include/tscore/Allocator.h
##########
@@ -152,7 +153,7 @@ template <class C> class ClassAllocator : public Allocator
@param tail pointer to be freed.
@param num_item of blocks to be freed.
*/
- void
+ std::enable_if_t<!Destruct_on_free, void>
Review comment:
I'm not a bit familiar with SFINAE. Let me clarify, which is your intention here when `Destruct_on_free == true` ?
a). do not define this function
or
b). make a compile error
Background: I got a `'enable_if' cannot be used to disabl
e this declaration` error when I tried setting the flag `true` like `ClassAllocator<UnixNetVConnection, true>`. If your intention is to do not declare/define this function, this error seems not expected.
```
/usr/local/opt/llvm/include/c++/v1/type_traits:542:78: error: no type named 'type' in 'std::__1::enable_if<false, void>'; 'enable_if' cannot be used to disabl
e this declaration
template <bool _Bp, class _Tp = void> using enable_if_t = typename enable_if<_Bp, _Tp>::type;
^~~
../../include/tscore/Allocator.h:156:8: note: in instantiation of template type alias 'enable_if_t' requested here
std::enable_if_t<!Destruct_on_free, void>
^ UnixNetProcessor.cc:334:10: note: in instantiation of template class 'ClassAllocator<UnixNetVConnection, true>' requested here
vc = THREAD_ALLOC_INIT(netVCAllocator, t);
^
/Users/masaori/src/github.com/apache/trafficserver-review/iocore/eventsystem/I_ProxyAllocator.h:114:35: note: expanded from macro 'THREAD_ALLOC_INIT'
#define THREAD_ALLOC_INIT(_a, _t) thread_alloc_init(::_a, _t->_a)
```
( clang version 11.0.0 )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-740230407
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-664585219
[approve ci FreeBSD]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-773659938
Yes, I think this change should be made on this PR. Otherwise, this PR will break master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-788581463
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-833919114
Marking for 9.1 because it makes merging easier. Especially picking over the h2 to origin branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras edited a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras edited a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-771132376
> My test build ran all weekend. The numbers (rps and average latency) look slightly better than its peer. Once the change is made in the CacheVC constructor to initialize the od member to nullptr, I'd endorse this change to be merged.
Should this change be made as a part of this PR @shinrich ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r585242214
##########
File path: iocore/eventsystem/I_ProxyAllocator.h
##########
@@ -45,84 +48,49 @@ struct ProxyAllocator {
ProxyAllocator() {}
};
-template <class C>
-inline C *
-thread_alloc(ClassAllocator<C> &a, ProxyAllocator &l)
+template <class CAlloc, typename... Args>
+typename CAlloc::Value_type *
+thread_alloc(CAlloc &a, ProxyAllocator &l, Args &&... args)
{
if (!cmd_disable_pfreelist && l.freelist) {
- C *v = (C *)l.freelist;
- l.freelist = *(C **)l.freelist;
+ void *v = l.freelist;
+ l.freelist = *reinterpret_cast<void **>(l.freelist);
--(l.allocated);
- *(void **)v = *(void **)&a.proto.typeObject;
- return v;
+ ::new (v) typename CAlloc::Value_type(std::forward<Args>(args)...);
+ return static_cast<typename CAlloc::Value_type *>(v);
}
- return a.alloc();
+ return a.alloc(std::forward<Args>(args)...);
}
-template <class C>
-inline C *
-thread_alloc_init(ClassAllocator<C> &a, ProxyAllocator &l)
-{
- if (!cmd_disable_pfreelist && l.freelist) {
- C *v = (C *)l.freelist;
- l.freelist = *(C **)l.freelist;
- --(l.allocated);
- memcpy((void *)v, (void *)&a.proto.typeObject, sizeof(C));
- return v;
- }
- return a.alloc();
-}
+void *thread_alloc(Allocator &a, ProxyAllocator &l);
+void thread_freeup(Allocator &a, ProxyAllocator &l);
-template <class C>
-inline void
-thread_free(ClassAllocator<C> &a, C *p)
-{
- a.free(p);
-}
+#if 1
-static inline void
-thread_free(Allocator &a, void *p)
-{
- a.free_void(p);
-}
-
-template <class C>
-inline void
-thread_freeup(ClassAllocator<C> &a, ProxyAllocator &l)
-{
- C *head = (C *)l.freelist;
- C *tail = (C *)l.freelist;
- size_t count = 0;
- while (l.freelist && l.allocated > thread_freelist_low_watermark) {
- tail = (C *)l.freelist;
- l.freelist = *(C **)l.freelist;
- --(l.allocated);
- ++count;
- }
+// Potentially empty varaiable arguments -- non-standard GCC way
+//
+#define THREAD_ALLOC(_a, _t, ...) thread_alloc(::_a, _t->_a, ##__VA_ARGS__)
+#define THREAD_ALLOC_INIT(_a, _t, ...) thread_alloc(::_a, _t->_a, ##__VA_ARGS__)
- if (unlikely(count == 1)) {
- a.free(tail);
- } else if (count > 0) {
- a.free_bulk(head, tail, count);
- }
+#else
- ink_assert(l.allocated >= thread_freelist_low_watermark);
-}
+// Potentially empty varaiable arguments -- Standard way
+//
+#define THREAD_ALLOC(_a, _t, ...) thread_alloc(::_a, _t->_a __VA_OPT__(, ) __VA_ARGS__)
Review comment:
note: We can't go with this "Standard" way until C++20. (`__VA_OPT__` is introduced by C++20)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r537836872
##########
File path: include/tscore/Allocator.h
##########
@@ -152,7 +153,7 @@ template <class C> class ClassAllocator : public Allocator
@param tail pointer to be freed.
@param num_item of blocks to be freed.
*/
- void
+ std::enable_if_t<!Destruct_on_free, void>
Review comment:
OK, this is much improved now. The conditionally enabled functions were not necessary and have been eliminated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-842507102
Cherry-picked to v9.1.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r536189482
##########
File path: include/tscore/Allocator.h
##########
@@ -152,7 +153,7 @@ template <class C> class ClassAllocator : public Allocator
@param tail pointer to be freed.
@param num_item of blocks to be freed.
*/
- void
+ std::enable_if_t<!Destruct_on_free, void>
Review comment:
Yes this is a Standard library template that uses "substitution failure is not a error". if Destruct_of_free is false, the member function will not be defined for the instantiation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r567086211
##########
File path: include/tscore/Allocator.h
##########
@@ -108,29 +109,39 @@ class Allocator
ink_freelist_madvise_init(&this->fl, name, element_size, chunk_size, alignment, advice);
}
+ // Dummies
+ void
+ destroy_if_enabled(void *)
+ {
+ }
+ Allocator &
+ raw()
+ {
+ return *this;
+ }
+
protected:
InkFreeList *fl;
};
/**
- Allocator for Class objects. It uses a prototype object to do
- fast initialization. Prototype of the template class is created
- when the fast allocator is created. This is instantiated with
- default (no argument) constructor. Constructor is not called for
- the allocated objects. Instead, the prototype is just memory
- copied onto the new objects. This is done for performance reasons.
+ Allocator for Class objects.
*/
-template <class C> class ClassAllocator : public Allocator
+template <class C, bool Destruct_on_free_ = false> class ClassAllocator : private Allocator
{
public:
- /** Allocates objects of the templated type. */
+ using Value_type = C;
+ static bool const Destruct_on_free = Destruct_on_free_;
+
+ /** Allocates objects of the templated type. Arguments are forwarded to the constructor for the object. */
+ template <typename... Args>
C *
- alloc()
+ alloc(Args &&... args)
{
void *ptr = ink_freelist_new(this->fl);
- memcpy(ptr, (void *)&this->proto.typeObject, sizeof(C));
+ ::new (ptr) C(std::forward<Args>(args)...);
Review comment:
Running with ASAN seems to initialize the allocated memory to a highly probably "bad" pattern. Once I fixed the cacheVC, the rest of the system seems to be running fine. At least this explains why we didn't see the CacheVC::od problem before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-775566984
> Yes, I think this change should be made on this PR. Otherwise, this PR will break master.
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras closed pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras closed pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-773659938
Yes, I think this change should be made on this PR. Otherwise, this PR will break master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r585247843
##########
File path: iocore/eventsystem/I_ProxyAllocator.h
##########
@@ -45,84 +48,49 @@ struct ProxyAllocator {
ProxyAllocator() {}
};
-template <class C>
-inline C *
-thread_alloc(ClassAllocator<C> &a, ProxyAllocator &l)
+template <class CAlloc, typename... Args>
+typename CAlloc::Value_type *
+thread_alloc(CAlloc &a, ProxyAllocator &l, Args &&... args)
{
if (!cmd_disable_pfreelist && l.freelist) {
- C *v = (C *)l.freelist;
- l.freelist = *(C **)l.freelist;
+ void *v = l.freelist;
+ l.freelist = *reinterpret_cast<void **>(l.freelist);
--(l.allocated);
- *(void **)v = *(void **)&a.proto.typeObject;
- return v;
+ ::new (v) typename CAlloc::Value_type(std::forward<Args>(args)...);
+ return static_cast<typename CAlloc::Value_type *>(v);
}
- return a.alloc();
+ return a.alloc(std::forward<Args>(args)...);
}
-template <class C>
-inline C *
-thread_alloc_init(ClassAllocator<C> &a, ProxyAllocator &l)
-{
- if (!cmd_disable_pfreelist && l.freelist) {
- C *v = (C *)l.freelist;
- l.freelist = *(C **)l.freelist;
- --(l.allocated);
- memcpy((void *)v, (void *)&a.proto.typeObject, sizeof(C));
- return v;
- }
- return a.alloc();
-}
+void *thread_alloc(Allocator &a, ProxyAllocator &l);
Review comment:
Bit off-topic, but could you include `tscore/Allocator.h` or add the forward declaration of `Allocator`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras edited a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras edited a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-771132376
> My test build ran all weekend. The numbers (rps and average latency) look slightly better than its peer. Once the change is made in the CacheVC constructor to initialize the od member to nullptr, I'd endorse this change to be merged.
Should this change be made as a part of this PR @shinrich ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-789338631
> Note above that Susan ran this on a production proxy and saw no loss of performance.
OK, let's ship this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-680365433
[approve ci docs]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-770929832
My test build ran all weekend. The numbers (rps and average latency) look slightly better than its peer. Once the change is made in the CacheVC constructor to initialize the od member to nullptr, I'd endorse this change to be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#discussion_r567014393
##########
File path: include/tscore/Allocator.h
##########
@@ -108,29 +109,39 @@ class Allocator
ink_freelist_madvise_init(&this->fl, name, element_size, chunk_size, alignment, advice);
}
+ // Dummies
+ void
+ destroy_if_enabled(void *)
+ {
+ }
+ Allocator &
+ raw()
+ {
+ return *this;
+ }
+
protected:
InkFreeList *fl;
};
/**
- Allocator for Class objects. It uses a prototype object to do
- fast initialization. Prototype of the template class is created
- when the fast allocator is created. This is instantiated with
- default (no argument) constructor. Constructor is not called for
- the allocated objects. Instead, the prototype is just memory
- copied onto the new objects. This is done for performance reasons.
+ Allocator for Class objects.
*/
-template <class C> class ClassAllocator : public Allocator
+template <class C, bool Destruct_on_free_ = false> class ClassAllocator : private Allocator
{
public:
- /** Allocates objects of the templated type. */
+ using Value_type = C;
+ static bool const Destruct_on_free = Destruct_on_free_;
+
+ /** Allocates objects of the templated type. Arguments are forwarded to the constructor for the object. */
+ template <typename... Args>
C *
- alloc()
+ alloc(Args &&... args)
{
void *ptr = ink_freelist_new(this->fl);
- memcpy(ptr, (void *)&this->proto.typeObject, sizeof(C));
+ ::new (ptr) C(std::forward<Args>(args)...);
Review comment:
"proto" is a data member of the class allocator, and I think they are always statically allocated. So the raw memory for proto will be zeroed out at load time before the proto object is constructed by placement new. We could zero the memory here before calling placement new. If we don't, there is a risk of seeing further crashes with this change, like the the Susan found.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-740230407
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-680365433
[approve ci docs]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-788574872
@bryancall Any performance differences on your benchmark?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-770929832
My test build ran all weekend. The numbers (rps and average latency) look slightly better than its peer. Once the change is made in the CacheVC constructor to initialized the od member to nullptr, I'd endorse this change to be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras merged pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras merged pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-769465552
Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #6241: Make Allocator.h less silly (no creepy "proto" object).
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #6241:
URL: https://github.com/apache/trafficserver/pull/6241#issuecomment-789008682
> @bryancall Any performance differences on your benchmark?
Note above that Susan ran this on a production proxy and saw no loss of performance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org