docs

README.rst

@@ -90,6 +90,16 @@ error.
   because the ESI level 0 thread can send available data to the client
   concurrently to other parallel ESI threads.
 
+  Serial mode may sensibly be used to reduce overhead and the number
+  of threads required without relevant drawbacks
+
+  - at esi_level > 0
+
+  - when the vcl author knows that all objects included by the current
+    request will be cacheable and thus highly likely cause cache hits.
+
+  Also see the discussion below for more detail.
+
 * ``thread``, requires *bool* argument
 
   Whether we always request a new thread for includes, default is
@@ -113,24 +123,42 @@ error.
   the client concurrently. As soon as ESI object processing is
   complete, the respective thread will be returned to the thread pool
   and become available for any other varnish task (except for the
-  request for esi_level 0, which will wait for completion of the
-  entire ESI request, sending data to the client in parallel).
+  request for esi_level 0, which _has_ to wait for completion of the
+  entire ESI request anyway and will send data to the client in the
+  meantime).
 
   With this setting to ``true`` (the default), this is what happens
-  always, but a thread for processing of includes may not be
-  immediately available and thus may have to wait if the thread pool
-  is not sufficiently large.
+  always, but a thread may not be immediately available if the thread
+  pool is not sufficiently sized for the current load and thus the
+  include request may have to be queued.
 
   With this setting to ``false``, include processing happens in the
   same thread as if ``serial`` mode had been activated, but only in
-  the case when there is no new thread available. While this may sound
-  like the more sensible option at first, we did not make this the
-  default for two reasons:
-
-  - except for ESI level 0, the current thread will become available
-    as soon as ESI processing has completed
-
-    *XXX slink to continue*
+  the case where there is no new thread available. While this may
+  sound like the more sensible option at first, we did not make this
+  the default for the following reasons:
+
+  - Before completion of the ESI processing, the subtree below it is
+    not yet available for delivery to the client because additional
+    VDPs behind pesi cannot be called from a different thread.
+
+  - While processing of the include may take an arbitrarily long time
+    (for example because it requires a lengthy backend fetch), we know
+    that the ESI object is fully available in the stevedore (and
+    usually in memory already) when we parse an include because
+    streaming is not supported for ESI. So we know that completing the
+    processing of the current ESI object will be quick, while
+    descending into a subtree may be take a long time.
+
+  - Except for ESI level 0, the current thread will become available
+    as soon as ESI processing has completed.
+
+  - The thread herder may breed new threads and other threads may
+    terminate, so queuing a thread momentarily is not a bad thing per
+    se.
+
+  In short, keeping ``thread`` at the default ``true`` should be the
+  right option, the alternative exists just in case.
 
 .. _vmod_pesi.version:
 

src/vdp_pesi.vcc

@@ -76,6 +76,16 @@ error.
   because the ESI level 0 thread can send available data to the client
   concurrently to other parallel ESI threads.
 
+  Serial mode may sensibly be used to reduce overhead and the number
+  of threads required without relevant drawbacks
+
+  - at esi_level > 0
+
+  - when the vcl author knows that all objects included by the current
+    request will be cacheable and thus highly likely cause cache hits.
+
+  Also see the discussion below for more detail.
+
 * ``thread``, requires *bool* argument
 
   Whether we always request a new thread for includes, default is
@@ -99,24 +109,42 @@ error.
   the client concurrently. As soon as ESI object processing is
   complete, the respective thread will be returned to the thread pool
   and become available for any other varnish task (except for the
-  request for esi_level 0, which will wait for completion of the
-  entire ESI request, sending data to the client in parallel).
+  request for esi_level 0, which _has_ to wait for completion of the
+  entire ESI request anyway and will send data to the client in the
+  meantime).
 
   With this setting to ``true`` (the default), this is what happens
-  always, but a thread for processing of includes may not be
-  immediately available and thus may have to wait if the thread pool
-  is not sufficiently large.
+  always, but a thread may not be immediately available if the thread
+  pool is not sufficiently sized for the current load and thus the
+  include request may have to be queued.
 
   With this setting to ``false``, include processing happens in the
   same thread as if ``serial`` mode had been activated, but only in
-  the case when there is no new thread available. While this may sound
-  like the more sensible option at first, we did not make this the
-  default for two reasons:
-
-  - except for ESI level 0, the current thread will become available
-    as soon as ESI processing has completed
-
-    *XXX slink to continue*
+  the case where there is no new thread available. While this may
+  sound like the more sensible option at first, we did not make this
+  the default for the following reasons:
+
+  - Before completion of the ESI processing, the subtree below it is
+    not yet available for delivery to the client because additional
+    VDPs behind pesi cannot be called from a different thread.
+
+  - While processing of the include may take an arbitrarily long time
+    (for example because it requires a lengthy backend fetch), we know
+    that the ESI object is fully available in the stevedore (and
+    usually in memory already) when we parse an include because
+    streaming is not supported for ESI. So we know that completing the
+    processing of the current ESI object will be quick, while
+    descending into a subtree may be take a long time.
+
+  - Except for ESI level 0, the current thread will become available
+    as soon as ESI processing has completed.
+
+  - The thread herder may breed new threads and other threads may
+    terminate, so queuing a thread momentarily is not a bad thing per
+    se.
+
+  In short, keeping ``thread`` at the default ``true`` should be the
+  right option, the alternative exists just in case.
 
 $Function STRING version()