Updated Branches: refs/heads/master f390a8e41 -> c3a14aafa
TS-2126: Avoid unnecessary memory copy in LogHost::write() Now, in LogHost::write (LogBuffer *lb), Another new LogBuffer object will be allocated to copy the content of *lb* parameter. But *lb* will be deleted by the upper function, LogBufferManager::flush_buffers(), after LogHost::write() return. Obviously, the copy is unnecessary, and may cause memory fragmentation. Let's optimize it. Signed-off-by: Yunkai Zhang <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/c3a14aaf Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/c3a14aaf Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/c3a14aaf Branch: refs/heads/master Commit: c3a14aafaa1d98b305dc9260814cdae2885a2c60 Parents: f390a8e Author: Yunkai Zhang <[email protected]> Authored: Sat Aug 10 22:20:32 2013 +0800 Committer: Yunkai Zhang <[email protected]> Committed: Mon Aug 12 11:58:27 2013 +0800 ---------------------------------------------------------------------- CHANGES | 2 + proxy/logging/LogBuffer.h | 15 ++++++ proxy/logging/LogBufferSink.h | 6 ++- proxy/logging/LogCollationClientSM.cc | 8 +-- proxy/logging/LogFile.cc | 42 ++++++++++------ proxy/logging/LogFile.h | 14 +++++- proxy/logging/LogHost.cc | 81 ++++++++++++++++-------------- proxy/logging/LogHost.h | 11 ++-- proxy/logging/LogObject.cc | 3 +- 9 files changed, 119 insertions(+), 63 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 9a15449..8c1894f 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,8 @@ Changes with Apache Traffic Server 3.5.0 + *) [TS-2126] Avoid unnecessary memory copy in LogHost::write() + *) [TS-2130] pthread_setname_np() detection fails on various platforms. *)[ TS-2096] improve SSL certificate loading error messages http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogBuffer.h ---------------------------------------------------------------------- diff --git a/proxy/logging/LogBuffer.h b/proxy/logging/LogBuffer.h index 055adec..acf67fe 100644 --- a/proxy/logging/LogBuffer.h +++ b/proxy/logging/LogBuffer.h @@ -188,6 +188,21 @@ public: int write_to_len, long timestamp, long timestamp_us, unsigned buffer_version, LogFieldList * alt_fieldlist = NULL, char *alt_printf_str = NULL); + static void destroy(LogBuffer *lb) + { + int result, old_ref, new_ref; + + do { + old_ref = lb->m_references; + new_ref = old_ref - 1; + result = ink_atomic_cas(&lb->m_references, old_ref, new_ref); + } while(!result); + + ink_release_assert(new_ref >= 0); + + if (new_ref == 0) + delete lb; + } private: char *m_unaligned_buffer; // the unaligned buffer http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogBufferSink.h ---------------------------------------------------------------------- diff --git a/proxy/logging/LogBufferSink.h b/proxy/logging/LogBufferSink.h index 8df2e31..6b3972c 100644 --- a/proxy/logging/LogBufferSink.h +++ b/proxy/logging/LogBufferSink.h @@ -35,7 +35,11 @@ the network class LogBufferSink { public: - virtual int write(LogBuffer * buffer) = 0; + // + // The write_and_delete() function should be responsible for + // freeing memory pointed to by _buffer_ parameter. + // + virtual int write_and_delete(LogBuffer * buffer) = 0; virtual ~ LogBufferSink() { }; http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogCollationClientSM.cc ---------------------------------------------------------------------- diff --git a/proxy/logging/LogCollationClientSM.cc b/proxy/logging/LogCollationClientSM.cc index 6ce02b4..933bc38 100644 --- a/proxy/logging/LogCollationClientSM.cc +++ b/proxy/logging/LogCollationClientSM.cc @@ -663,7 +663,7 @@ LogCollationClientSM::client_send(int event, VIO * /* vio ATS_UNUSED */) // done with the buffer, delete it Debug("log-coll", "[%d]client::client_send - m_buffer_in_iocore[%p] to delete_list", m_id, m_buffer_in_iocore); - delete m_buffer_in_iocore; + LogBuffer::destroy(m_buffer_in_iocore); m_buffer_in_iocore = NULL; // switch back to client_send @@ -711,7 +711,8 @@ LogCollationClientSM::flush_to_orphan() Debug("log-coll", "[%d]client::flush_to_orphan - m_buffer_in_iocore to oprhan", m_id); // TODO: We currently don't try to make the log buffers handle little vs big endian. TS-1156. // m_buffer_in_iocore->convert_to_host_order(); - m_log_host->orphan_write_and_delete(m_buffer_in_iocore); + m_log_host->orphan_write(m_buffer_in_iocore); + LogBuffer::destroy(m_buffer_in_iocore); m_buffer_in_iocore = NULL; } // flush buffers in send_list to orphan @@ -719,7 +720,8 @@ LogCollationClientSM::flush_to_orphan() ink_assert(m_buffer_send_list != NULL); while ((log_buffer = m_buffer_send_list->get()) != NULL) { Debug("log-coll", "[%d]client::flush_to_orphan - send_list to orphan", m_id); - m_log_host->orphan_write_and_delete(log_buffer); + m_log_host->orphan_write(log_buffer); + LogBuffer::destroy(log_buffer); } // Now send_list is empty, let's update m_flow to ALLOW status http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogFile.cc ---------------------------------------------------------------------- diff --git a/proxy/logging/LogFile.cc b/proxy/logging/LogFile.cc index 29eb4d5..0a2e53f 100644 --- a/proxy/logging/LogFile.cc +++ b/proxy/logging/LogFile.cc @@ -472,37 +472,41 @@ LogFile::roll(long interval_start, long interval_end) } /*------------------------------------------------------------------------- - LogFile::write + LogFile::write_and_try_delete - Write the given LogBuffer data onto this file + Write the given LogBuffer data onto this file and + free the buffer memory according _need_delete_ parameter. -------------------------------------------------------------------------*/ int -LogFile::write(LogBuffer * lb) +LogFile::write_and_try_delete(LogBuffer * lb, bool need_delete) { + int bytes = 0; + int result = -1; + LogBufferHeader *buffer_header; + if (lb == NULL) { Note("Cannot write LogBuffer to LogFile %s; LogBuffer is NULL", m_name); return -1; } - LogBufferHeader *buffer_header = lb->header(); - if (buffer_header == NULL) { + if ((buffer_header = lb->header()) == NULL) { Note("Cannot write LogBuffer to LogFile %s; LogBufferHeader is NULL", m_name); - return -1; + goto done; } if (buffer_header->entry_count == 0) { // no bytes to write Note("LogBuffer with 0 entries for LogFile %s, nothing to write", m_name); - return 0; + result = 0; + goto done; } // make sure we're open & ready to write check_fd(); if (!is_open()) { - return -1; + goto done; } - int bytes = 0; if (m_file_format == BINARY_LOG) { // // Ok, now we need to write the binary buffer to the file, and we @@ -512,9 +516,14 @@ LogFile::write(LogBuffer * lb) // don't change between buffers), it's not worth trying to separate // out the buffer-dependent data from the buffer-independent data. // - bytes = ::write(m_fd, buffer_header, buffer_header->byte_count); - if (static_cast<uint32_t>(bytes) != buffer_header->byte_count) { - Warning("An error was encountered writing to %s: [tried %d, wrote %d, '%s']", m_name, buffer_header->byte_count, bytes, strerror(errno)); + while (static_cast<uint32_t>(bytes) < buffer_header->byte_count) { + int cnt = ::write(m_fd, buffer_header, buffer_header->byte_count); + if (cnt < 0) { + Error("An error was encountered writing to %s: [tried %d, wrote %d, '%s']", + m_name, buffer_header->byte_count, bytes, strerror(errno)); + break; + } + bytes += cnt; } } else if (m_file_format == ASCII_LOG || m_file_format == ASCII_PIPE) { @@ -527,7 +536,7 @@ LogFile::write(LogBuffer * lb) else { Note("Cannot write LogBuffer to LogFile %s; invalid file format: %d", m_name, m_file_format); - return -1; + goto done; } // @@ -540,7 +549,12 @@ LogFile::write(LogBuffer * lb) m_start_time = buffer_header->low_timestamp; m_end_time = buffer_header->high_timestamp; m_bytes_written += bytes; - return bytes; + result = bytes; + +done: + if (need_delete) + delete lb; + return result; } /*------------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogFile.h ---------------------------------------------------------------------- diff --git a/proxy/logging/LogFile.h b/proxy/logging/LogFile.h index 8d3a4a8..ddba682 100644 --- a/proxy/logging/LogFile.h +++ b/proxy/logging/LogFile.h @@ -141,7 +141,13 @@ public: LOG_FILE_FILESYSTEM_CHECKS_FAILED }; - int write(LogBuffer * lb); + inline int write(LogBuffer * lb) { + return write_and_try_delete(lb, false); + } + + inline int write_and_delete(LogBuffer * lb) { + return write_and_try_delete(lb, true); + } int roll(long interval_start, long interval_end); char *get_name() const { return m_name; } @@ -173,6 +179,12 @@ private: static int writeln(char *data, int len, int fd, const char *path); void read_metadata(); + // + // Write the given LogBuffer data onto this file and + // free the buffer memory according _need_delete_ parameter. + // + int write_and_try_delete(LogBuffer * lb, bool need_delete); + private: LogFileFormat m_file_format; char *m_name; http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogHost.cc ---------------------------------------------------------------------- diff --git a/proxy/logging/LogHost.cc b/proxy/logging/LogHost.cc index 7804f68..1f9ff0d 100644 --- a/proxy/logging/LogHost.cc +++ b/proxy/logging/LogHost.cc @@ -259,9 +259,15 @@ LogHost::create_orphan_LogFile_object() ats_free(name_buf); } +// +// write the buffer data to target host and try to +// delete it when its reference become zero. +// int -LogHost::write (LogBuffer *lb) +LogHost::write_and_try_delete (LogBuffer *lb) { + int result = -1; + if (lb == NULL) { Note("Cannot write LogBuffer to LogHost %s; LogBuffer is NULL", name()); return -1; @@ -270,11 +276,12 @@ LogHost::write (LogBuffer *lb) if (buffer_header == NULL) { Note("Cannot write LogBuffer to LogHost %s; LogBufferHeader is NULL", name()); - return -1; + goto done; } if (buffer_header->entry_count == 0) { // no bytes to write - return 0; + result = 0; + goto done; } #if !defined(IOCORE_LOG_COLLATION) @@ -283,9 +290,9 @@ LogHost::write (LogBuffer *lb) if (!connected(NOPING)) { if (!connect ()) { - Note("Cannot write LogBuffer to LogHost %s; not connected", - name()); - return orphan_write (lb); + Note("Cannot write LogBuffer to LogHost %s; not connected", name()); + result = orphan_write(lb); + goto done; } } @@ -301,26 +308,17 @@ LogHost::write (LogBuffer *lb) disconnect(); // TODO: We currently don't try to make the log buffers handle little vs big endian. TS-1156. // lb->convert_to_host_order (); - return orphan_write (lb); + result = orphan_write(lb); + goto done; } Debug("log-host","%d bytes sent to LogHost %s:%u", bytes_sent, name(), port()); SUM_DYN_STAT (log_stat_bytes_sent_to_network_stat, bytes_sent); - return bytes_sent; + result = bytes_sent; + goto done; #else // !defined(IOCORE_LOG_COLLATION) - // make a copy of our log_buffer - int buffer_header_size = buffer_header->byte_count; - LogBufferHeader *buffer_header_copy = - (LogBufferHeader*) NEW(new char[buffer_header_size]); - ink_assert(buffer_header_copy != NULL); - - memcpy(buffer_header_copy, buffer_header, buffer_header_size); - LogBuffer *lb_copy = NEW(new LogBuffer(lb->get_owner(), - buffer_header_copy)); - ink_assert(lb_copy != NULL); - // create a new collation client if necessary if (m_log_collation_client_sm == NULL) { m_log_collation_client_sm = NEW(new LogCollationClientSM(this)); @@ -328,18 +326,23 @@ LogHost::write (LogBuffer *lb) } // send log_buffer; orphan if necessary - int bytes_sent = m_log_collation_client_sm->send(lb_copy); - if (bytes_sent <= 0) { - orphan_write_and_delete(lb_copy); + result = m_log_collation_client_sm->send(lb); + if (result <= 0) { + result = orphan_write(lb); #if defined(LOG_BUFFER_TRACKING) - Debug("log-buftrak", "[%d]LogHost::write - orphan write complete", - lb_copy->header()->id); + Debug("log-buftrak", "[%d]LogHost::write_and_try_delete - orphan write complete", + lb->header()->id); #endif // defined(LOG_BUFFER_TRACKING) + goto done; } - return bytes_sent; + return result; #endif // !defined(IOCORE_LOG_COLLATION) + +done: + LogBuffer::destroy(lb); + return result; } int @@ -353,16 +356,6 @@ LogHost::orphan_write(LogBuffer * lb) } } -int -LogHost::orphan_write_and_delete(LogBuffer * lb) -{ - int bytes = orphan_write(lb); - // done with the buffer, delete it - delete lb; - lb = 0; - return bytes; -} - void LogHost::display(FILE * fd) { @@ -457,14 +450,26 @@ LogHostList::clear() } int -LogHostList::write(LogBuffer * lb) +LogHostList::write_and_delete(LogBuffer * lb) { int total_bytes = 0; - for (LogHost * host = first(); host; host = next(host)) { - int bytes = host->write(lb); + unsigned nr_host, nr; + + ink_release_assert(lb->m_references == 0); + + nr_host = nr = count(); + ink_atomic_increment(&lb->m_references, nr_host); + + for (LogHost * host = first(); host && nr; host = next(host)) { + int bytes = host->write_and_try_delete(lb); if (bytes > 0) total_bytes += bytes; + nr--; } + + if (nr_host == 0) + delete lb; + return total_bytes; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogHost.h ---------------------------------------------------------------------- diff --git a/proxy/logging/LogHost.h b/proxy/logging/LogHost.h index f0227e7..201ca93 100644 --- a/proxy/logging/LogHost.h +++ b/proxy/logging/LogHost.h @@ -33,7 +33,7 @@ class LogCollationClientSM; LogHost This object corresponds to a named log collation host. -------------------------------------------------------------------------*/ -class LogHost: public LogBufferSink +class LogHost { //#if defined(IOCORE_LOG_COLLATION) @@ -52,7 +52,11 @@ public: bool connected(bool ping); bool connect(); void disconnect(); - int write(LogBuffer * lb); + // + // write the buffer data to target host and try to + // delete it when its reference become zero. + // + int write_and_try_delete(LogBuffer * lb); char const* name() const { return m_name ? m_name : "UNKNOWN"; } IpAddr const& ip_addr() const { return m_ip; } @@ -69,7 +73,6 @@ private: void clear(); bool authenticated(); int orphan_write(LogBuffer * lb); - int orphan_write_and_delete(LogBuffer * lb); void create_orphan_LogFile_object(); private: @@ -108,7 +111,7 @@ public: void add(LogHost * host, bool copy = true); unsigned count(); void clear(); - int write(LogBuffer * lb); + int write_and_delete(LogBuffer * lb); LogHost *first() { return m_host_list.head; } LogHost *next(LogHost * here) { return (here->link).next; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c3a14aaf/proxy/logging/LogObject.cc ---------------------------------------------------------------------- diff --git a/proxy/logging/LogObject.cc b/proxy/logging/LogObject.cc index c4bcd5d..b61eaa1 100644 --- a/proxy/logging/LogObject.cc +++ b/proxy/logging/LogObject.cc @@ -59,8 +59,7 @@ LogBufferManager::flush_buffers(LogBufferSink *sink) { int flushed = 0; while ((b = new_q.pop())) { b->update_header_data(); - sink->write(b); - delete b; + sink->write_and_delete(b); ink_atomic_increment(&_num_flush_buffers, -1); flushed++; }
