thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dave Watson (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-1442) TNonblockingServer: Refactor to allow multiple IO Threads
Date Thu, 08 Dec 2011 18:56:40 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-1442?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165432#comment-13165432
] 

Dave Watson commented on THRIFT-1442:
-------------------------------------

Ah thanks for the link, I've updated my test environment to include those flags.  

I've attached patch to fix that particular warning, however my environment (gcc 4.6) I get
a lot more.  Here's some random warning fixes to make mine build, not sure if you need any
of them

Fix unused variable if a struct contains no members:
{code}
diff --git a/compiler/cpp/src/generate/t_cpp_generator.cc b/compiler/cpp/src/generate/t_cpp_generator.cc
index 0354a5f..e923be1 100755
--- a/compiler/cpp/src/generate/t_cpp_generator.cc
+++ b/compiler/cpp/src/generate/t_cpp_generator.cc
@@ -1505,13 +1505,16 @@ void t_cpp_generator::generate_struct_swap(ofstream& out, t_struct*
tstruct) {
   out <<
     indent() << "using ::std::swap;" << endl;
 
+  // Prevent unused variable warnings
+  indent(out) << "(void)a;" << endl;
+  indent(out) << "(void)b;" << endl;
+
   bool has_nonrequired_fields = false;
   const vector<t_field*>& fields = tstruct->get_members();
   for (vector<t_field*>::const_iterator f_iter = fields.begin();
        f_iter != fields.end();
        ++f_iter) {
     t_field *tfield = *f_iter;
-    t_type *ttype = get_true_type(tfield->get_type());
 
     if (tfield->get_req() != t_field::T_REQUIRED) {
       has_nonrequired_fields = true;
{code}


I'm not sure what is correct here for unnamed_oprot_seqid regarding THRIFT-1165, it still
warns for me:

{code}
diff --git a/compiler/cpp/src/generate/t_cpp_generator.cc b/compiler/cpp/src/generate/t_cpp_generator.cc
index 0354a5f..6974f84 100755
--- a/compiler/cpp/src/generate/t_cpp_generator.cc
+++ b/compiler/cpp/src/generate/t_cpp_generator.cc
@@ -3164,8 +3164,7 @@ void t_cpp_generator::generate_process_function(t_service* tservice,
       out <<
         indent() << "template <class Protocol_>" << endl;
     }
-    const bool unnamed_oprot_seqid = tfunction->is_oneway() &&
-      !(gen_templates_ && !specialized);
+    const bool unnamed_oprot_seqid = tfunction->is_oneway();
     out <<
       "void " << tservice->get_name() << "Processor" << class_suffix
<< "::" <<
       "process_" << tfunction->get_name() << "(" <<
{code}

depending on ifdefs, milliseconds var is unused:

{code}
diff --git a/lib/cpp/src/concurrency/Mutex.cpp b/lib/cpp/src/concurrency/Mutex.cpp
index 0cfa0ad..332d415 100644
--- a/lib/cpp/src/concurrency/Mutex.cpp
+++ b/lib/cpp/src/concurrency/Mutex.cpp
@@ -154,6 +154,7 @@ class Mutex::impl {
     PROFILE_MUTEX_NOT_LOCKED();
     return false;
 #else
+    (void)milliseconds;
     // If pthread_mutex_timedlock isn't supported, the safest thing to do
     // is just do a nonblocking trylock.
     return trylock();
{code}

printf seems to be wrong type:

{code}
diff --git a/lib/cpp/src/transport/TFileTransport.cpp b/lib/cpp/src/transport/TFileTransport.cpp
index 47dc328..351f950 100644
--- a/lib/cpp/src/transport/TFileTransport.cpp
+++ b/lib/cpp/src/transport/TFileTransport.cpp
@@ -68,6 +68,7 @@ using namespace apache::thrift::concurrency;
 #define CLOCK_REALTIME 0
 static int clock_gettime(int clk_id /*ignored*/, struct timespec *tp) {
   struct timeval now;
+  (void)clk_id;
 
   int rv = gettimeofday(&now, NULL);
   if (rv != 0) {
@@ -783,7 +784,7 @@ bool TFileTransport::isEventCorrupted() {
   } else if( ((offset_ + readState_.bufferPtr_ - 4)/chunkSize_) !=
              ((offset_ + readState_.bufferPtr_ + readState_.event_->eventSize_ - 1)/chunkSize_)
) {
     // 3. size indicates that event crosses chunk boundary
-    T_ERROR("Read corrupt event. Event crosses chunk boundary. Event size:%u  Offset:%lu",
+    T_ERROR("Read corrupt event. Event crosses chunk boundary. Event size:%u  Offset:%lli",
             readState_.event_->eventSize_,
             (offset_ + readState_.bufferPtr_ + 4));
 
@@ -825,7 +826,7 @@ void TFileTransport::performRecovery() {
       readState_.resetState(readState_.lastDispatchPtr_);
       currentEvent_ = NULL;
       char errorMsg[1024];
-      sprintf(errorMsg, "TFileTransport: log file corrupted at offset: %lu",
+      sprintf(errorMsg, "TFileTransport: log file corrupted at offset: %lli",
               (offset_ + readState_.lastDispatchPtr_));
               
       GlobalOutput(errorMsg);
{code}
                
> TNonblockingServer: Refactor to allow multiple IO Threads
> ---------------------------------------------------------
>
>                 Key: THRIFT-1442
>                 URL: https://issues.apache.org/jira/browse/THRIFT-1442
>             Project: Thrift
>          Issue Type: Improvement
>          Components: C++ - Library
>            Reporter: Dave Watson
>            Assignee: Dave Watson
>            Priority: Minor
>             Fix For: 0.9
>
>         Attachments: 0001-TNonblockingServer-Refactor-to-allow-multiple-IO-thr.patch,
nonblocking_unsigned.patch
>
>
> From 04fc8cbc24c64e1b68a23a1df2c46056785c269d Mon Sep 17 00:00:00 2001
> From: Mark Rabkin <mrabkin@fb.com>
> Date: Tue, 18 May 2010 22:16:09 +0000
> Subject: [PATCH 01/56] TNonblockingServer: Refactor to allow multiple IO
>  threads, not just one
> davejwatson: This diff ads multiple IO threads to TNonblocking Server. 
> We use it extensively, it's pretty well tested other than merge errors.  
> This diff reverts THRIFT-1184, allowing re-use of an existing
> event base.  With multiple IO threads, re-using a single event base doesn't
> make sense, so this seemed ok
> Summary:
> The diff creates multiple IO threads at startup -- the number of threads in
> this diff is fixed at server start and cannot change for simplicity.  The
> first thread (id = 0) also doubles as the listen/accept thread, so there is
> still only a single thread doing accepts.  The other IO threads listen only
> on their notification pipe and the actual connection sockets.
> Also, for simplicity, each accepted connection is simply assigned in a
> round-robin fashion to the next IO thread and lives on that IO thread
> permanently.
> Note that there are only trivial changes to TConnection to get it to work, so
> most of the tricky state transitions and buffer management are unchanged.
> There is still a single worker pool for processing tasks, so that code is
> unchanged as well.
> The trickiest part of the diff requiring the most careful review is the new
> synchronization code in the TNonblockingServer to manage the connection stack
> and counters of active/inactive connections.  We now lock a mutex when
> incrementing/decrementing server counters, which is less than ideal for
> extremely high-QPS servers -- should I switch to atomic ops?
> One important change here is that while connections are created and initialized
> by the listen thread (IO thread #0), they may now be assigned to an event_base
> owned by a different IO thread.  To work safely, TConnection::init() no longer
> calls setFlags() - instead, it immediately calls TConnection::notifyIOThread().
> This results in a notification-fd event in the correct event base, which then
> calls TConnection::transition() which sets up correct read flags.  This means
> that a TConnection now calls notifyIOThread() once upon creation and assignment
> to its event_base, and thereafter after each processing call completes.
> TNonblocking server: Allow high-priority scheduler for IO threads
> Summary:
> Adds a boolean option to TNonblockingServer to use sched_setscheduler() to set
> a high scheduling priority for the IO thread(s) -- this is a POSIX api and
> should be safe on most operating systems.
> Please let me know if this is a known terrible idea, but we're experimenting
> to see if this helps the situation where we have 40 worker threads and 1 IO
> thread and the IO thread doesn't get scheduled nearly often enough.
> Reviewers: dreiss,edhall,putivsky
> Test Plan:  Need to work with Ed to run his capacity-loadtesting scripts to verify performance.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message