From 59af44bf174d4c00e6f3713e899c254359efbc9b Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Tue, 27 Oct 2020 12:11:22 -0600 Subject: [PATCH] [Security Solution] critical pref bug with browser fields reducer ## Summary How to test this performance issue? Ensure that you add extra mappings of mapping-test-1k-block-1, 2, 3,4, etc.. that I have included within your advanced settings like below to a Kibana space: ```ts apm-*-transaction*, auditbeat-*, endgame-*, filebeat-*, logs-*, packetbeat-*, winlogbeat-*, mapping-test-1k-block-1, mapping-test-1k-block-2, mapping-test-1k-block-3, mapping-test-1k-block-4 ``` Screen Shot 2020-10-26 at 11 35 12 AM Modify these lines to add manual perf lines like so for the before numbers: file: x-pack/plugins/security_solution/public/common/containers/source/index.tsx ```ts export const getBrowserFields = memoizeOne( (_title: string, fields: IndexField[]): BrowserFields => { console.time('getBrowserFields'); // Start time const value = fields && fields.length > 0 ? fields.reduce( (accumulator: BrowserFields, field: IndexField) => set([field.category, 'fields', field.name], field, accumulator), {} ) : {}; console.timeEnd('getBrowserFields'); // End time return value; }, // Update the value only if _title has changed (newArgs, lastArgs) => newArgs[0] === lastArgs[0] ); ``` Then load any of the SIEM webpages or create a detection engine rule and change around your input indexes. Perf of this before the fix is going to show up in your dev tools console output like this: ```ts getBrowserFields: 7875.5009765625 ms ``` Now, checkout this branch and change the code to be like so for the perf test of the mutatious version for after: file: x-pack/plugins/security_solution/public/common/containers/source/index.tsx ```ts /** * HOT Code path where the fields can be 16087 in length or larger. This is * VERY mutatious on purpose to improve the performance of the transform. */ export const getBrowserFields = memoizeOne( (_title: string, fields: IndexField[]): BrowserFields => { console.time('getBrowserFields'); // Start time // Adds two dangerous casts to allow for mutations within this function type DangerCastForMutation = Record; type DangerCastForBrowserFieldsMutation = Record< string, Omit & { fields: Record } >; // We mutate this instead of using lodash/set to keep this as fast as possible const value = fields.reduce((accumulator, field) => { if (accumulator[field.category] == null) { (accumulator as DangerCastForMutation)[field.category] = {}; } if (accumulator[field.category].fields == null) { accumulator[field.category].fields = {}; } if (accumulator[field.category].fields[field.name] == null) { (accumulator[field.category].fields as DangerCastForMutation)[field.name] = {}; } accumulator[field.category].fields[field.name] = (field as unknown) as BrowserField; return accumulator; }, {}); console.timeEnd('getBrowserFields'); // End time return value; }, // Update the value only if _title has changed (newArgs, lastArgs) => newArgs[0] === lastArgs[0] ); ``` Re-load any and all pages and you should see this now: ```ts getBrowserFields: 11.258056640625 ms ``` ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --- .../common/containers/source/index.test.tsx | 30 +++++++++++++++++ .../public/common/containers/source/index.tsx | 33 ++++++++++++++----- 2 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 x-pack/plugins/security_solution/public/common/containers/source/index.test.tsx diff --git a/x-pack/plugins/security_solution/public/common/containers/source/index.test.tsx b/x-pack/plugins/security_solution/public/common/containers/source/index.test.tsx new file mode 100644 index 000000000000..e81b52f28151 --- /dev/null +++ b/x-pack/plugins/security_solution/public/common/containers/source/index.test.tsx @@ -0,0 +1,30 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { IndexField } from '../../../../common/search_strategy/index_fields'; +import { getBrowserFields } from '.'; +import { mockBrowserFields, mocksSource } from './mock'; + +describe('source/index.tsx', () => { + describe('getBrowserFields', () => { + test('it returns an empty object given an empty array', () => { + const fields = getBrowserFields('title 1', []); + expect(fields).toEqual({}); + }); + + test('it returns the same input with the same title', () => { + getBrowserFields('title 1', []); + // Since it is memoized it will return the same output which is empty object given 'title 1' a second time + const fields = getBrowserFields('title 1', mocksSource.indexFields as IndexField[]); + expect(fields).toEqual({}); + }); + + test('it transforms input into output as expected', () => { + const fields = getBrowserFields('title 2', mocksSource.indexFields as IndexField[]); + expect(fields).toEqual(mockBrowserFields); + }); + }); +}); diff --git a/x-pack/plugins/security_solution/public/common/containers/source/index.tsx b/x-pack/plugins/security_solution/public/common/containers/source/index.tsx index 2cc1c75015e0..47e550b2ced0 100644 --- a/x-pack/plugins/security_solution/public/common/containers/source/index.tsx +++ b/x-pack/plugins/security_solution/public/common/containers/source/index.tsx @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -import { set } from '@elastic/safer-lodash-set/fp'; import { keyBy, pick, isEmpty, isEqual, isUndefined } from 'lodash/fp'; import memoizeOne from 'memoize-one'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; @@ -55,15 +54,31 @@ export const getIndexFields = memoizeOne( (newArgs, lastArgs) => newArgs[0] === lastArgs[0] && newArgs[1].length === lastArgs[1].length ); +/** + * HOT Code path where the fields can be 16087 in length or larger. This is + * VERY mutatious on purpose to improve the performance of the transform. + */ export const getBrowserFields = memoizeOne( - (_title: string, fields: IndexField[]): BrowserFields => - fields && fields.length > 0 - ? fields.reduce( - (accumulator: BrowserFields, field: IndexField) => - set([field.category, 'fields', field.name], field, accumulator), - {} - ) - : {}, + (_title: string, fields: IndexField[]): BrowserFields => { + // Adds two dangerous casts to allow for mutations within this function + type DangerCastForMutation = Record; + type DangerCastForBrowserFieldsMutation = Record< + string, + Omit & { fields: Record } + >; + + // We mutate this instead of using lodash/set to keep this as fast as possible + return fields.reduce((accumulator, field) => { + if (accumulator[field.category] == null) { + (accumulator as DangerCastForMutation)[field.category] = {}; + } + if (accumulator[field.category].fields == null) { + accumulator[field.category].fields = {}; + } + accumulator[field.category].fields[field.name] = (field as unknown) as BrowserField; + return accumulator; + }, {}); + }, // Update the value only if _title has changed (newArgs, lastArgs) => newArgs[0] === lastArgs[0] );