siddharthteotia commented on a change in pull request #6886:
URL: https://github.com/apache/incubator-pinot/pull/6886#discussion_r629762731
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -37,21 +36,51 @@ public InstanceResponseOperator(Operator combinedOperator) {
@Override
protected InstanceResponseBlock getNextBlock() {
- ThreadTimer mainThreadTimer = new ThreadTimer();
- mainThreadTimer.start();
-
+ long startWallClockTimeNs = System.nanoTime();
IntermediateResultsBlock intermediateResultsBlock =
(IntermediateResultsBlock) _operator.nextBlock();
InstanceResponseBlock instanceResponseBlock = new
InstanceResponseBlock(intermediateResultsBlock);
DataTable dataTable = instanceResponseBlock.getInstanceResponseDataTable();
+ long endWallClockTimeNs = System.nanoTime();
+
+ long multipleThreadCpuTimeNs =
intermediateResultsBlock.getExecutionThreadCpuTimeNs();
+ long totalWallClockTimeNs = endWallClockTimeNs - startWallClockTimeNs;
- mainThreadTimer.stop();
+ int numServerThreads = intermediateResultsBlock.getNumServerThreads();
long totalThreadCpuTimeNs =
- intermediateResultsBlock.getExecutionThreadCpuTimeNs() +
mainThreadTimer.getThreadTimeNs();
+ calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs,
numServerThreads);
+
dataTable.getMetadata().put(MetadataKey.THREAD_CPU_TIME_NS.getName(),
String.valueOf(totalThreadCpuTimeNs));
return instanceResponseBlock;
}
+ /*
+ * Calculate totalThreadCpuTimeNs based on totalWallClockTimeNs,
multipleThreadCpuTimeNs, and numServerThreads.
+ * System activities time such as OS paging, GC, context switching are not
captured by totalThreadCpuTimeNs.
+ * For example, let's divide query processing into 4 phases:
+ * - phase 1: single thread preparing. Time used: T1
+ * - phase 2: N threads processing segments in parallel, each thread use
time T2
+ * - phase 3: GC/OS paging. Time used: T3
+ * - phase 4: single thread merging intermediate results blocks. Time used:
T4
+ *
+ * Then we have following equations:
+ * - singleThreadCpuTimeNs = T1 + T4
+ * - multipleThreadCpuTimeNs = T2 * N
+ * - totalWallClockTimeNs = T1 + T2 + T3 + T4 = singleThreadCpuTimeNs + T2 +
T3
+ * - totalThreadCpuTimeNsWithoutSystemActivities = T1 + T2 * N + T4 =
singleThreadCpuTimeNs + T2 * N
+ * - systemActivitiesTimeNs = T3 = (totalWallClockTimeNs -
totalThreadCpuTimeNsWithoutSystemActivities) + T2 * (N - 1)
+ *
+ * Thus:
+ * totalThreadCpuTimeNsWithSystemActivities =
totalThreadCpuTimeNsWithoutSystemActivities + systemActivitiesTimeNs
+ * = totalThreadCpuTimeNsWithoutSystemActivities + T3
+ * = totalThreadCpuTimeNsWithoutSystemActivities + (totalWallClockTimeNs -
totalThreadCpuTimeNsWithoutSystemActivities) + T2 * (N - 1)
+ * = totalWallClockTimeNs + T2 * (N - 1)
+ * = totalWallClockTimeNs + (multipleThreadCpuTimeNs / N) * (N - 1)
+ */
+ public static long calTotalThreadCpuTimeNs(long totalWallClockTimeNs, long
multipleThreadCpuTimeNs, int numServerThreads) {
+ return totalWallClockTimeNs + multipleThreadCpuTimeNs * (numServerThreads
- 1) / numServerThreads;
+ }
Review comment:
We should use parentheses here for precedence I think. Also, we should
use rounding. It should be :
```
double a = multipleThreadCpuTimeNs / (double)numServerThreads ;
// we should not get 0 above and that's why casting to double
double b = a * (numThreads - 1) =>
long c = Math.round ((double)wallClock + b)
return c
```
Please make sure unit tests cover all scenarios
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
