Skip to content

Commit

Permalink
[Security Solution] critical pref bug with browser fields reducer
Browse files Browse the repository at this point in the history
## 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
```

<img width="1331" alt="Screen Shot 2020-10-26 at 11 35 12 AM" src="https://user-images.githubusercontent.com/1151048/97224071-3a904a00-1796-11eb-8870-6de35fc2f115.png">

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<BrowserFields>(
            (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<string, {}>;
    type DangerCastForBrowserFieldsMutation = Record<
      string,
      Omit<BrowserField, 'fields'> & { fields: Record<string, BrowserField> }
    >;

    // We mutate this instead of using lodash/set to keep this as fast as possible
    const value = fields.reduce<DangerCastForBrowserFieldsMutation>((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
  • Loading branch information
FrankHassanabad authored Oct 27, 2020
1 parent 8b1ff4c commit 59af44b
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<BrowserFields>(
(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<string, {}>;
type DangerCastForBrowserFieldsMutation = Record<
string,
Omit<BrowserField, 'fields'> & { fields: Record<string, BrowserField> }
>;

// We mutate this instead of using lodash/set to keep this as fast as possible
return fields.reduce<DangerCastForBrowserFieldsMutation>((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]
);
Expand Down

0 comments on commit 59af44b

Please sign in to comment.