Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

POC: to batch plugin responses this has reduced webworker scripting by 2 seconds #37762

Draft
wants to merge 2 commits into
base: release
Choose a base branch
from
+88 −11
Draft

POC: to batch plugin responses this has reduced webworker scripting by 2 seconds #37762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
534fd11
batching updateAction Data basically batching the results of plugin r…
vsvamsi1 Nov 26, 2024
9bd05ce
batch updates trigger value
vsvamsi1 Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/client/src/ce/actions/evaluationActionsList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ export const EVALUATE_REDUX_ACTIONS = [
// Buffer
ReduxActionTypes.BUFFERED_ACTION,
// Generic
ReduxActionTypes.TRIGGER_EVAL,
// ReduxActionTypes.TRIGGER_EVAL,
ReduxActionTypes.TRIGGER_EVAL_BATCH,
];
// Topics used for datasource and query form evaluations
export const FORM_EVALUATION_REDUX_ACTIONS = [
Expand Down
1 change: 1 addition & 0 deletions app/client/src/ce/constants/ReduxActionConstants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ const EvaluationActionTypes = {
BUFFERED_ACTION: "BUFFERED_ACTION",
CLEAR_CACHE: "CLEAR_CACHE",
TRIGGER_EVAL: "TRIGGER_EVAL",
TRIGGER_EVAL_BATCH: "TRIGGER_EVAL_BATCH",
UPDATE_ACTION_DATA: "UPDATE_ACTION_DATA",
SET_ACTIVE_EDITOR_FIELD: "SET_ACTIVE_EDITOR_FIELD",
RESET_ACTIVE_EDITOR_FIELD: "RESET_ACTIVE_EDITOR_FIELD",
Expand Down
87 changes: 81 additions & 6 deletions app/client/src/sagas/ActionExecution/PluginActionSaga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ import {
all,
call,
delay,
fork,
put,
race,
select,
take,
takeEvery,
takeLatest,
} from "redux-saga/effects";
import * as Sentry from "@sentry/react";
import type { updateActionDataPayloadType } from "actions/pluginActionActions";
import {
clearActionResponse,
executePageLoadActions,
Expand Down Expand Up @@ -1674,10 +1675,9 @@ function* softRefreshActionsSaga() {
yield put({ type: ReduxActionTypes.SWITCH_ENVIRONMENT_SUCCESS });
}

function* handleUpdateActionData(
action: ReduxAction<updateActionDataPayloadType>,
) {
const { actionDataPayload, parentSpan } = action.payload;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function* handleUpdateActionData(action: any) {
Comment on lines +1678 to +1679
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add proper TypeScript types instead of using 'any'.

Define an interface for the action parameter instead of using 'any' to maintain type safety.

-function* handleUpdateActionData(action: any) {
+interface UpdateActionDataPayload {
+  actionDataPayload: Array<{
+    entityName: string;
+    dataPath: string;
+    data: unknown;
+  }>;
+  parentSpan?: OtlpSpan;
+}
+function* handleUpdateActionData(action: UpdateActionDataPayload) {

Committable suggestion skipped: line range outside the PR's diff.

const { actionDataPayload, parentSpan } = action;

yield call(
evalWorker.request,
Expand All @@ -1690,6 +1690,77 @@ function* handleUpdateActionData(
}
}

function* captureActionsWithinPeriodTriggers() {
while (true) {
const buffer = []; // Initialize a new buffer for each batch
const endTime = Date.now() + 10000;
// eslint-disable-next-line prefer-const

while (Date.now() < endTime) {
try {
// Use a non-blocking `take` to capture actions within the period

const { action } = yield race({
action: take(ReduxActionTypes.TRIGGER_EVAL),
del: delay(1000),
});

if (!action) continue;

buffer.push(action);
} catch (e) {
// Handle errors if needed
}
}

// After the time period, dispatch the collected actions
if (buffer.length > 0) {
yield put({
type: ReduxActionTypes.TRIGGER_EVAL_BATCH,
});
}
}
}
Comment on lines +1693 to +1723
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve performance and error handling.

The function has two issues:

  1. The busy-waiting loop can cause high CPU usage
  2. Empty catch block could hide important errors
 function* captureActionsWithinPeriodTriggers() {
   while (true) {
     const buffer = [];
-    const endTime = Date.now() + 10000;
+    const BATCH_WINDOW = 10000;
+    const batchStart = Date.now();

-    while (Date.now() < endTime) {
+    while (Date.now() - batchStart < BATCH_WINDOW) {
       try {
         const { action } = yield race({
           action: take(ReduxActionTypes.TRIGGER_EVAL),
-          del: delay(1000),
+          timeout: delay(Math.max(0, BATCH_WINDOW - (Date.now() - batchStart))),
         });

         if (!action) continue;
         buffer.push(action);
       } catch (e) {
-        // Handle errors if needed
+        log.error("Error in captureActionsWithinPeriodTriggers:", e);
       }
     }

     if (buffer.length > 0) {
       yield put({
         type: ReduxActionTypes.TRIGGER_EVAL_BATCH,
       });
     }
   }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function* captureActionsWithinPeriodTriggers() {
while (true) {
const buffer = []; // Initialize a new buffer for each batch
const endTime = Date.now() + 10000;
// eslint-disable-next-line prefer-const
while (Date.now() < endTime) {
try {
// Use a non-blocking `take` to capture actions within the period
const { action } = yield race({
action: take(ReduxActionTypes.TRIGGER_EVAL),
del: delay(1000),
});
if (!action) continue;
buffer.push(action);
} catch (e) {
// Handle errors if needed
}
}
// After the time period, dispatch the collected actions
if (buffer.length > 0) {
yield put({
type: ReduxActionTypes.TRIGGER_EVAL_BATCH,
});
}
}
}
function* captureActionsWithinPeriodTriggers() {
while (true) {
const buffer = [];
const BATCH_WINDOW = 10000;
const batchStart = Date.now();
while (Date.now() - batchStart < BATCH_WINDOW) {
try {
const { action } = yield race({
action: take(ReduxActionTypes.TRIGGER_EVAL),
timeout: delay(Math.max(0, BATCH_WINDOW - (Date.now() - batchStart))),
});
if (!action) continue;
buffer.push(action);
} catch (e) {
log.error("Error in captureActionsWithinPeriodTriggers:", e);
}
}
if (buffer.length > 0) {
yield put({
type: ReduxActionTypes.TRIGGER_EVAL_BATCH,
});
}
}
}


// Use a channel to queue all actions

function* captureActionsWithinPeriod() {
while (true) {
const buffer = []; // Initialize a new buffer for each batch
const endTime = Date.now() + 10000;
let parentSpan;
// eslint-disable-next-line prefer-const

while (Date.now() < endTime) {
try {
// Use a non-blocking `take` to capture actions within the period

const { action } = yield race({
action: take(ReduxActionTypes.UPDATE_ACTION_DATA),
del: delay(1000),
});

if (!action) continue;

const { actionDataPayload } = action.payload;

parentSpan = action.payload.parentSpan;
buffer.push(...actionDataPayload);
} catch (e) {
// Handle errors if needed
}
}

// After the time period, dispatch the collected actions
if (buffer.length > 0) {
yield fork(handleUpdateActionData, {
parentSpan,
actionDataPayload: buffer,
});
}
}
}

Comment on lines +1725 to +1763
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor looping mechanism to prevent high CPU usage.

The inner loop while (Date.now() < endTime) can cause high CPU utilization due to continuous execution. Consider using a more efficient approach that waits for actions or a timeout without busy-waiting.

Apply this diff to refactor the function:

 function* captureActionsWithinPeriod() {
   while (true) {
+    const buffer = []; // Initialize a new buffer for each batch
+    let parentSpan;
+    const startTime = Date.now();
+    const endTime = startTime + 10000;
+
+    while (Date.now() < endTime) {
       try {
-        const { action } = yield race({
-          action: take(ReduxActionTypes.UPDATE_ACTION_DATA),
-          del: delay(1000),
-        });
+        const { action, timeout } = yield race({
+          action: take(ReduxActionTypes.UPDATE_ACTION_DATA),
+          timeout: delay(endTime - Date.now()),
+        });

-        if (!action) continue;
+        if (timeout) {
+          break;
+        }

         const { actionDataPayload } = action.payload;
         parentSpan = action.payload.parentSpan;
         buffer.push(...actionDataPayload);
       } catch (e) {
         // Handle errors if needed
       }
     }

     // After the time period, dispatch the collected actions
     if (buffer.length > 0) {
       yield fork(handleUpdateActionData, {
         parentSpan,
         actionDataPayload: buffer,
       });
     }
   }
 }

Committable suggestion skipped: line range outside the PR's diff.

export function* watchPluginActionExecutionSagas() {
yield all([
takeLatest(ReduxActionTypes.RUN_ACTION_REQUEST, runActionSaga),
Expand All @@ -1703,6 +1774,10 @@ export function* watchPluginActionExecutionSagas() {
),
takeLatest(ReduxActionTypes.PLUGIN_SOFT_REFRESH, softRefreshActionsSaga),
takeEvery(ReduxActionTypes.EXECUTE_JS_UPDATES, makeUpdateJSCollection),
takeEvery(ReduxActionTypes.UPDATE_ACTION_DATA, handleUpdateActionData),
takeEvery(ReduxActionTypes.START_EVALUATION, captureActionsWithinPeriod),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent multiple instances of captureActionsWithinPeriod.

Using takeEvery with captureActionsWithinPeriod may start multiple saga instances if START_EVALUATION is dispatched repeatedly. To ensure only a single instance runs, consider using takeLeading or starting the saga once.

Apply this diff to adjust the saga watcher:

 export function* watchPluginActionExecutionSagas() {
   yield all([
     takeLatest(ReduxActionTypes.RUN_ACTION_REQUEST, runActionSaga),
     takeLatest(
       ReduxActionTypes.RUN_ACTION_SHORTCUT_REQUEST,
       runActionShortcutSaga,
     ),
     takeLatest(
       ReduxActionTypes.EXECUTE_PAGE_LOAD_ACTIONS,
       executePageLoadActionsSaga,
     ),
     takeLatest(ReduxActionTypes.PLUGIN_SOFT_REFRESH, softRefreshActionsSaga),
     takeEvery(ReduxActionTypes.EXECUTE_JS_UPDATES, makeUpdateJSCollection),
-    takeEvery(ReduxActionTypes.START_EVALUATION, captureActionsWithinPeriod),
+    takeLeading(ReduxActionTypes.START_EVALUATION, captureActionsWithinPeriod),
   ]);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
takeEvery(ReduxActionTypes.START_EVALUATION, captureActionsWithinPeriod),
export function* watchPluginActionExecutionSagas() {
yield all([
takeLatest(ReduxActionTypes.RUN_ACTION_REQUEST, runActionSaga),
takeLatest(
ReduxActionTypes.RUN_ACTION_SHORTCUT_REQUEST,
runActionShortcutSaga,
),
takeLatest(
ReduxActionTypes.EXECUTE_PAGE_LOAD_ACTIONS,
executePageLoadActionsSaga,
),
takeLatest(ReduxActionTypes.PLUGIN_SOFT_REFRESH, softRefreshActionsSaga),
takeEvery(ReduxActionTypes.EXECUTE_JS_UPDATES, makeUpdateJSCollection),
takeLeading(ReduxActionTypes.START_EVALUATION, captureActionsWithinPeriod),
]);
}

takeEvery(
ReduxActionTypes.START_EVALUATION,
captureActionsWithinPeriodTriggers,
),
]);
}
8 changes: 4 additions & 4 deletions app/client/src/workers/Evaluation/fns/utils/TriggerEmitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@ const logsHandler = deferredBatchedActionHandler((batchedData) =>

TriggerEmitter.on(BatchKey.process_logs, logsHandler);

const storeUpdatesHandler = priorityBatchedActionHandler((batchedData) =>
WorkerMessenger.ping({
const storeUpdatesHandler = priorityBatchedActionHandler((batchedData) => {
return WorkerMessenger.ping({
method: MAIN_THREAD_ACTION.PROCESS_STORE_UPDATES,
data: batchedData,
}),
);
});
});
Comment on lines +85 to +90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Consider adding explicit returns to other batched handlers for consistency

Found similar handlers in TriggerEmitter.ts that would benefit from explicit returns:

  • defaultTriggerHandler - currently no return statement
  • Handler in jsVariableUpdatesHandler - currently no return statement
  • Log processing handler - currently no return statement
🔗 Analysis chain

LGTM! The explicit return improves clarity.

The addition of the explicit return statement for WorkerMessenger.ping enhances code clarity and enables proper promise chaining if needed by callers.

Let's verify if other similar handlers need the same treatment:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other handlers that might benefit from explicit returns
ast-grep --pattern 'const $_Handler = priorityBatchedActionHandler((batchedData) => {
  WorkerMessenger.ping({
    $$$
  })
})'

Length of output: 129

Script:

#!/bin/bash
# Let's try a broader search for priorityBatchedActionHandler usage
rg -A 5 "priorityBatchedActionHandler" 

# Also search for similar patterns with WorkerMessenger.ping
rg -A 5 "WorkerMessenger.ping"

Length of output: 4869


TriggerEmitter.on(BatchKey.process_store_updates, storeUpdatesHandler);

Expand Down
Loading