From 17c359a5c42673816debe081dd22c9f7e32a4e7c Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Thu, 2 Dec 2021 16:27:20 -0700 Subject: [PATCH 1/4] [Reporting] Fix performance of CSV generation --- .../generate_csv/generate_csv.ts | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts index 76172da3e99cf..0d00123566354 100644 --- a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts +++ b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts @@ -242,7 +242,7 @@ export class CsvGenerator { /* * Format a Datatable into rows of CSV content */ - private generateRows( + private async generateRows( columns: string[], table: Datatable, builder: MaxSizeStringBuilder, @@ -250,18 +250,27 @@ export class CsvGenerator { settings: CsvExportSettings ) { this.logger.debug(`Building ${table.rows.length} CSV data rows...`); + + const asyncGenerateRow = async (dataTableRow: Record): Promise => { + return new Promise((resolve) => { + setImmediate(() => { + resolve( + columns + .map((f) => ({ column: f, data: dataTableRow[f] })) + .map(this.formatCellValues(formatters)) + .map(this.escapeValues(settings)) + .join(settings.separator) + '\n' + ); + }); + }); + }; + for (const dataTableRow of table.rows) { if (this.cancellationToken.isCancelled()) { break; } - const row = - columns - .map((f) => ({ column: f, data: dataTableRow[f] })) - .map(this.formatCellValues(formatters)) - .map(this.escapeValues(settings)) - .join(settings.separator) + '\n'; - + const row = await asyncGenerateRow(dataTableRow); if (!builder.tryAppend(row)) { this.logger.warn(`Max Size Reached after ${this.csvRowCount} rows.`); this.maxSizeReached = true; @@ -377,7 +386,7 @@ export class CsvGenerator { } const formatters = this.getFormatters(table); - this.generateRows(columns, table, builder, formatters, settings); + await this.generateRows(columns, table, builder, formatters, settings); // update iterator currentRecord += table.rows.length; From ebacdbbb0640cc0b6577475dbc9be3b814b8d99d Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Fri, 3 Dec 2021 14:11:36 -0700 Subject: [PATCH 2/4] use a for loop with 1 operation instead of 3 chained maps --- .../generate_csv/generate_csv.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts index 0d00123566354..8697d148e63ac 100644 --- a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts +++ b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts @@ -254,13 +254,15 @@ export class CsvGenerator { const asyncGenerateRow = async (dataTableRow: Record): Promise => { return new Promise((resolve) => { setImmediate(() => { - resolve( - columns - .map((f) => ({ column: f, data: dataTableRow[f] })) - .map(this.formatCellValues(formatters)) - .map(this.escapeValues(settings)) - .join(settings.separator) + '\n' - ); + const row: string[] = []; + const format = this.formatCellValues(formatters); + const escape = this.escapeValues(settings); + + for (const column of columns) { + row.push(escape(format({ column, data: dataTableRow[column] }))); + } + + resolve(row.join(settings.separator) + '\n'); }); }); }; From d04b1e11ef41fbd2fa7855daa49d758344f879a2 Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Fri, 3 Dec 2021 14:16:59 -0700 Subject: [PATCH 3/4] do without the callback --- .../generate_csv/generate_csv.ts | 31 +++++++------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts index 8697d148e63ac..6b1597dd30259 100644 --- a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts +++ b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts @@ -250,30 +250,23 @@ export class CsvGenerator { settings: CsvExportSettings ) { this.logger.debug(`Building ${table.rows.length} CSV data rows...`); - - const asyncGenerateRow = async (dataTableRow: Record): Promise => { - return new Promise((resolve) => { - setImmediate(() => { - const row: string[] = []; - const format = this.formatCellValues(formatters); - const escape = this.escapeValues(settings); - - for (const column of columns) { - row.push(escape(format({ column, data: dataTableRow[column] }))); - } - - resolve(row.join(settings.separator) + '\n'); - }); - }); - }; - for (const dataTableRow of table.rows) { if (this.cancellationToken.isCancelled()) { break; } - const row = await asyncGenerateRow(dataTableRow); - if (!builder.tryAppend(row)) { + // comment about why this is here + await new Promise(setImmediate); + + const rowDefinition: string[] = []; + const format = this.formatCellValues(formatters); + const escape = this.escapeValues(settings); + + for (const column of columns) { + rowDefinition.push(escape(format({ column, data: dataTableRow[column] }))); + } + + if (!builder.tryAppend(rowDefinition.join(settings.separator) + '\n')) { this.logger.warn(`Max Size Reached after ${this.csvRowCount} rows.`); this.maxSizeReached = true; if (this.cancellationToken) { From 0d124e22db40a68bcc8b593efbfe21f6a446a4dc Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Fri, 3 Dec 2021 14:26:32 -0700 Subject: [PATCH 4/4] update comment --- .../generate_csv/generate_csv.ts | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts index 6b1597dd30259..4f2eb9109c958 100644 --- a/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts +++ b/x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts @@ -255,7 +255,27 @@ export class CsvGenerator { break; } - // comment about why this is here + /* + * Intrinsically, generating the rows is a synchronous process. Awaiting + * on a setImmediate call here partititions what could be a very long and + * CPU-intenstive synchronous process into an asychronous process. This + * give NodeJS to process other asychronous events that wait on the Event + * Loop. + * + * See: https://nodejs.org/en/docs/guides/dont-block-the-event-loop/ + * + * It's likely this creates a lot of context switching, and adds to the + * time it would take to generate the CSV. There are alternatives to the + * chosen performance solution: + * + * 1. Partition the synchronous process with fewer partitions, by using + * the loop counter to call setImmediate only every N amount of rows. + * Testing is required to see what the best N value for most data will + * be. + * + * 2. Use a C++ add-on to generate the CSV using the Node Worker Pool + * instead of using the Event Loop + */ await new Promise(setImmediate); const rowDefinition: string[] = [];