Skip to content

Commit

Permalink
[Discover] Improve data grid render performance (flattenHit and `fo…
Browse files Browse the repository at this point in the history
…rmatHit`) (#192333)

## Summary

This PR improves the performance of two important utilities used by
Discover: `flattenHit` and `formatHit`. It may not have a big impact on
typical Discover use cases, but these two functions can account for a
significant portion of the Discover data grid render time when viewing
documents with many fields (e.g. our `many_fields` performance journey)

### Improvements
- `formatHit` - Used for formatting document column cell values
- Previously we would call `formatFieldValue` on every field value in
every document even though we only show up to a specific number of field
values in each cell (configurable, but 200 by default), which can be
very expensive in some cases. In this PR we only call `formatFieldValue`
on field values that will be shown to the user, reducing the total calls
from 343,850 to 10,000 for the `many_fields` dataset.
- There are also some optimizations to the looping logic and much fewer
allocations than before.
- `ownKeys` (`flattenHit`) - Used to ensure document keys are
consistently ordered when looping over them
- We currently use a proxy around the `DataTableRecord.flattened` object
to override the `ownKeys` function and ensure document keys are always
consistently ordered when looping over them. The issue is that the
sorting logic gets called every time `DataTableRecord.flattened` keys
are looped over or accessed via `Object.keys`, `Object.entries`, etc.,
which happens often. In this PR the sorted keys are cached for each
`DataTableRecord.flattened` instance until one of its properties are
either set or deleted.
- The actual sorting logic has also been optimized with less casting,
removing calls to `localeCompare`, and caching.

### Tests
3 runs each, navigating to Discover with `many_fields` data. There's
some overlap between `formatHit` and `ownKeys` since `formatHit` loops
over `DataTableRecord.flattened` keys and triggers `ownKeys`, which is
why the total render difference doesn't match the sum of differences for
both methods.

#### Main

||Total render|`formatHit`|`ownKeys` (`flattenHit`)|
|---|---|---|---|
||4748 ms|1117 ms|1728 ms|
||4804 ms|1111 ms|1757 ms|
||4722 ms|1117 ms|1744 ms|
|**AVG**|4758 ms|1115 ms|1743 ms|

<img width="2168" alt="main"
src="https://github.com/user-attachments/assets/dcd718fc-9da7-460d-a045-b88b23db5e1b">

#### This PR

||Total render|`formatHit`|`ownKeys` (`flattenHit`)|
|---|---|---|---|
||3060 ms|159 ms|578 ms|
||3203 ms|154 ms|626 ms|
||3358 ms|160 ms| 623 ms|
|**AVG**|3207 ms|158 ms|609 ms|

<img width="2168" alt="pr"
src="https://github.com/user-attachments/assets/c2c9df43-6b0b-426a-826d-231160e3f6ef">

#### Average differences

|Total render|`formatHit`|`ownKeys` (`flattenHit`)|
|---|---|---|
|1551 ms|957 ms|1134 ms|

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
davismcphee authored Sep 12, 2024
1 parent 897cca0 commit 09af367
Show file tree
Hide file tree
Showing 8 changed files with 166 additions and 60 deletions.
7 changes: 6 additions & 1 deletion packages/kbn-data-service/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@
export type { GetConfigFn } from './src/types';
export { UI_SETTINGS } from './src/constants';
export { getEsQueryConfig } from './src/es_query';
export { tabifyDocs, flattenHit } from './src/search/tabify';
export {
tabifyDocs,
flattenHit,
getFlattenedFieldsComparator,
type FlattenedFieldsComparator,
} from './src/search/tabify';
7 changes: 6 additions & 1 deletion packages/kbn-data-service/src/search/tabify/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,9 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

export { tabifyDocs, flattenHit } from './tabify_docs';
export {
tabifyDocs,
flattenHit,
getFlattenedFieldsComparator,
type FlattenedFieldsComparator,
} from './tabify_docs';
69 changes: 59 additions & 10 deletions packages/kbn-data-service/src/search/tabify/tabify_docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ function flattenAccum(
* @param indexPattern The index pattern for the requested index if available.
* @param params Parameters how to flatten the hit
*/
export function flattenHit(hit: Hit, indexPattern?: DataView, params?: TabifyDocsOptions) {
export function flattenHit(
hit: Hit,
indexPattern?: DataView,
params?: TabifyDocsOptions & { flattenedFieldsComparator?: FlattenedFieldsComparator }
) {
const flat = {} as Record<string, any>;

flattenAccum(flat, hit.fields || {}, '', indexPattern, params);
Expand Down Expand Up @@ -147,28 +151,73 @@ export function flattenHit(hit: Hit, indexPattern?: DataView, params?: TabifyDoc

// Use a proxy to make sure that keys are always returned in a specific order,
// so we have a guarantee on the flattened order of keys.
return makeProxy(flat, indexPattern);
return makeProxy(flat, indexPattern, params?.flattenedFieldsComparator);
}

function makeProxy(flat: Record<string, any>, indexPattern?: DataView) {
function comparator(a: string | symbol, b: string | symbol) {
const aIsMeta = indexPattern?.metaFields?.includes(String(a));
const bIsMeta = indexPattern?.metaFields?.includes(String(b));
export const getFlattenedFieldsComparator = (indexPattern?: DataView) => {
const metaFields = new Set(indexPattern?.metaFields);
const lowerMap = new Map<string, string>();
let aLower: string | undefined;
let bLower: string | undefined;

const compareLower = (a: string, b: string) => {
aLower = lowerMap.get(a);
if (aLower === undefined) {
aLower = a.toLowerCase();
lowerMap.set(a, aLower);
}
bLower = lowerMap.get(b);
if (bLower === undefined) {
bLower = b.toLowerCase();
lowerMap.set(b, bLower);
}
return aLower < bLower ? -1 : aLower > bLower ? 1 : 0;
};

return (a: string | symbol, b: string | symbol) => {
if (typeof a === 'symbol' || typeof b === 'symbol') {
return 0;
}
const aIsMeta = metaFields.has(a);
const bIsMeta = metaFields.has(b);
if (aIsMeta && bIsMeta) {
return String(a).localeCompare(String(b));
return compareLower(a, b);
}
if (aIsMeta) {
return 1;
}
if (bIsMeta) {
return -1;
}
return String(a).localeCompare(String(b));
}
return compareLower(a, b);
};
};

export type FlattenedFieldsComparator = ReturnType<typeof getFlattenedFieldsComparator>;

function makeProxy(
flat: Record<string, any>,
indexPattern?: DataView,
flattenedFieldsComparator?: FlattenedFieldsComparator
) {
let cachedKeys: Array<string | symbol> | undefined;

return new Proxy(flat, {
defineProperty: (...args) => {
cachedKeys = undefined;
return Reflect.defineProperty(...args);
},
deleteProperty: (...args) => {
cachedKeys = undefined;
return Reflect.deleteProperty(...args);
},
ownKeys: (target) => {
return Reflect.ownKeys(target).sort(comparator);
if (!cachedKeys) {
cachedKeys = Reflect.ownKeys(target).sort(
flattenedFieldsComparator ?? getFlattenedFieldsComparator(indexPattern)
);
}
return cachedKeys;
},
});
}
Expand Down
17 changes: 13 additions & 4 deletions packages/kbn-discover-utils/src/utils/build_data_record.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
*/

import type { DataView } from '@kbn/data-views-plugin/common';
import { flattenHit } from '@kbn/data-service';
import {
type FlattenedFieldsComparator,
flattenHit,
getFlattenedFieldsComparator,
} from '@kbn/data-service';
import type { DataTableRecord, EsHitRecord } from '../types';
import { getDocId } from './get_doc_id';

Expand All @@ -21,12 +25,16 @@ import { getDocId } from './get_doc_id';
export function buildDataTableRecord(
doc: EsHitRecord,
dataView?: DataView,
isAnchor?: boolean
isAnchor?: boolean,
options?: { flattenedFieldsComparator?: FlattenedFieldsComparator }
): DataTableRecord {
return {
id: getDocId(doc),
raw: doc,
flattened: flattenHit(doc, dataView, { includeIgnoredValues: true }),
flattened: flattenHit(doc, dataView, {
includeIgnoredValues: true,
flattenedFieldsComparator: options?.flattenedFieldsComparator,
}),
isAnchor,
};
}
Expand All @@ -45,8 +53,9 @@ export function buildDataTableRecordList<T extends DataTableRecord = DataTableRe
dataView?: DataView;
processRecord?: (record: DataTableRecord) => T;
}): DataTableRecord[] {
const buildRecordOptions = { flattenedFieldsComparator: getFlattenedFieldsComparator(dataView) };
return records.map((doc) => {
const record = buildDataTableRecord(doc, dataView);
const record = buildDataTableRecord(doc, dataView, undefined, buildRecordOptions);
return processRecord ? processRecord(record) : record;
});
}
103 changes: 68 additions & 35 deletions packages/kbn-discover-utils/src/utils/format_hit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ import type {
} from '../types';
import { formatFieldValue } from './format_value';

// We use a special type here allowing formattedValue to be undefined because
// we want to avoid formatting values which will not be shown to users since
// it can be costly, and instead only format the ones which will be rendered
type PartialHitPair = [
fieldDisplayName: string,
formattedValue: string | undefined,
fieldName: string | null
];

const formattedHitCache = new WeakMap<
EsHitRecord,
{ formattedHit: FormattedHit; maxEntries: number }
Expand All @@ -41,54 +50,78 @@ export function formatHit(
fieldFormats: FieldFormatsStart
): FormattedHit {
const cached = formattedHitCache.get(hit.raw);

if (cached && cached.maxEntries === maxEntries) {
return cached.formattedHit;
}

const highlights = hit.raw.highlight ?? {};
// Flatten the object using the flattenHit implementation we use across Discover for flattening documents.
const flattened = hit.flattened;
const renderedPairs: PartialHitPair[] = [];
const otherPairs: PartialHitPair[] = [];

const highlightPairs: FormattedHit = [];
const sourcePairs: FormattedHit = [];

// Add each flattened field into the corresponding array for highlighted or other fields,
// depending on whether the original hit had a highlight for it. That way we can later
// put highlighted fields first in the document summary.
Object.entries(flattened).forEach(([key, val]) => {
// Add each flattened field into the corresponding array for rendered or other pairs,
// depending on whether the original hit had a highlight for it. That way we can ensure
// highlighted fields are shown first in the document summary.
for (const key of Object.keys(flattened)) {
// Retrieve the (display) name of the fields, if it's a mapped field on the data view
const field = dataView.fields.getByName(key);
const displayKey = field?.displayName;
const pairs = highlights[key] ? highlightPairs : sourcePairs;
// Format the raw value using the regular field formatters for that field
const formattedValue = formatFieldValue(val, hit.raw, fieldFormats, dataView, field);
// If the field was a mapped field, we validate it against the fieldsToShow list, if not
// we always include it into the result.
const pairs = highlights[key] ? renderedPairs : otherPairs;

// If the field is a mapped field, we first check if it should be shown,
// if not we always include it into the result.
if (displayKey) {
if (shouldShowFieldHandler(key)) {
pairs.push([displayKey, formattedValue, key]);
pairs.push([displayKey, undefined, key]);
}
} else {
pairs.push([key, formattedValue, key]);
pairs.push([key, undefined, key]);
}
});
const pairs = [...highlightPairs, ...sourcePairs];
const formatted =
// If document has more formatted fields than configured via MAX_DOC_FIELDS_DISPLAYED we cut
// off additional fields and instead show a summary how many more field exists.
pairs.length <= maxEntries
? pairs
: [
...pairs.slice(0, maxEntries),
[
i18n.translate('discover.formatHit.moreFields', {
defaultMessage: 'and {count} more {count, plural, one {field} other {fields}}',
values: { count: pairs.length - maxEntries },
}),
'',
null,
] as const,
];
formattedHitCache.set(hit.raw, { formattedHit: formatted, maxEntries });
return formatted;
}

const totalLength = renderedPairs.length + otherPairs.length;

// Truncate the renderedPairs if it exceeds the maxEntries,
// otherwise fill it up with otherPairs until it reaches maxEntries
if (renderedPairs.length > maxEntries) {
renderedPairs.length = maxEntries;
} else if (renderedPairs.length < maxEntries && otherPairs.length) {
for (let i = 0; i < otherPairs.length && renderedPairs.length < maxEntries; i++) {
renderedPairs.push(otherPairs[i]);
}
}

// Now format only the values which will be shown to the user
for (const pair of renderedPairs) {
const key = pair[2]!;

// Format the raw value using the regular field formatters for that field
pair[1] = formatFieldValue(
flattened[key],
hit.raw,
fieldFormats,
dataView,
dataView.getFieldByName(key)
);
}

// If document has more formatted fields than configured via MAX_DOC_FIELDS_DISPLAYED we cut
// off additional fields and instead show a summary how many more field exists.
if (totalLength > maxEntries) {
renderedPairs.push([
i18n.translate('discover.formatHit.moreFields', {
defaultMessage: 'and {count} more {count, plural, one {field} other {fields}}',
values: { count: totalLength - maxEntries },
}),
'',
null,
]);
}

const formattedHit = renderedPairs as FormattedHit;

formattedHitCache.set(hit.raw, { formattedHit, maxEntries });

return formattedHit;
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import { estypes } from '@elastic/elasticsearch';
import { lastValueFrom } from 'rxjs';
import { ISearchSource, EsQuerySortValue, SortDirection } from '@kbn/data-plugin/public';
import { buildDataTableRecord } from '@kbn/discover-utils';
import { buildDataTableRecordList } from '@kbn/discover-utils';
import type { DataTableRecord } from '@kbn/discover-utils/types';
import type { SearchResponseWarning } from '@kbn/search-response-warnings';
import { RequestAdapter } from '@kbn/inspector-plugin/common';
Expand Down Expand Up @@ -99,11 +99,11 @@ export async function fetchHitsInInterval(

const { rawResponse } = await lastValueFrom(fetch$);
const dataView = searchSource.getField('index');
const rows = rawResponse.hits?.hits.map((hit) =>
profilesManager.resolveDocumentProfile({
record: buildDataTableRecord(hit, dataView!),
})
);
const rows = buildDataTableRecordList({
records: rawResponse.hits?.hits,
dataView,
processRecord: (record) => profilesManager.resolveDocumentProfile({ record }),
});
const interceptedWarnings: SearchResponseWarning[] = [];
services.data.search.showWarnings(adapter, (warning) => {
interceptedWarnings.push(warning);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ export interface DiscoverSidebarResponsiveProps {
* Desktop: Sidebar view, all elements are visible
* Mobile: Data view selector is visible and a button to trigger a flyout with all elements
*/

export function DiscoverSidebarResponsive(props: DiscoverSidebarResponsiveProps) {
const [unifiedFieldListSidebarContainerApi, setUnifiedFieldListSidebarContainerApi] =
useState<UnifiedFieldListSidebarContainerApi | null>(null);
Expand Down
10 changes: 7 additions & 3 deletions src/plugins/discover/public/embeddable/initialize_fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@ import { BehaviorSubject, combineLatest, lastValueFrom, switchMap, tap } from 'r

import { KibanaExecutionContext } from '@kbn/core/types';
import {
buildDataTableRecord,
buildDataTableRecordList,
SEARCH_EMBEDDABLE_TYPE,
SEARCH_FIELDS_FROM_SOURCE,
SORT_DEFAULT_ORDER_SETTING,
} from '@kbn/discover-utils';
import { EsHitRecord } from '@kbn/discover-utils/types';
import { isOfAggregateQueryType, isOfQueryType } from '@kbn/es-query';
import { i18n } from '@kbn/i18n';
import { RequestAdapter } from '@kbn/inspector-plugin/common';
Expand Down Expand Up @@ -203,7 +202,12 @@ export function initializeFetch({

return {
warnings: interceptedWarnings,
rows: resp.hits.hits.map((hit) => buildDataTableRecord(hit as EsHitRecord, dataView)),
rows: buildDataTableRecordList({
records: resp.hits.hits,
dataView,
processRecord: (record) =>
discoverServices.profilesManager.resolveDocumentProfile({ record }),
}),
hitCount: resp.hits.total as number,
fetchContext,
};
Expand Down

0 comments on commit 09af367

Please sign in to comment.