Improve migration perf #99773

mshustov · 2021-05-11T10:52:01Z

Summary

Part of #92933
While working on SO migrations I noticed a couple of low-hanging fruits improving migration performance.
Testing performed on https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrationsv2/integration_tests/archives/7.7.2_xpack_100k_obj.zip data set as we needed any reproducible source of a large number of SO.
Most of the time Kibana spends on transforming SO POJO (considering I run the test locally with the minimal network overhead) so I removed a few expensive operations:

SavedObjectMigrationContext is created once and frozen instead of being created for every SO object
do not clone State on every model call. State shouldn't be mutated during migration by design. See the discussion below Improve migration perf #99773 (comment)

It allowed Kibana to reduce migration time for the 100k test data set by ~16% from 99.6sec to 83.536sec.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-05-11T10:52:03Z

Pinging @elastic/kibana-core (Team:Core)

pgayvallet · 2021-05-11T11:19:37Z

src/core/server/saved_objects/migrationsv2/model.ts

-  let stateP: State = cloneDeep(currentState);
+  let stateP: State = currentState;


Are we sure our state workflow is pure enough for this to not have any side effect? (should be but still asking)

This might be a red herring:

https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrationsv2/migrations_state_action_machine.ts#L50

We are risking that any step might call stateP.logs.push(). So far, I found that we are using everywhere stateP.logs = [...stateP.logs, {...}], so we should be fine...

Are we sure our state workflow is pure enough for this to not have any side effect? (should be but still asking)

@pgayvallet I'm not quite sure cloneDeep usage is justified here.
Mutations made during model execution will be returned from the model and State will be cloned with mutations during the next model call.
The only difference is the OldState mutation won't affect NewState shape. But AFAIK SavedObjectMigrationFn is the only valid case for mutations and we already clone the doc during migrateAndConvert call.

kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts

Line 245 in 2f6a3ce

const clonedDoc = _.cloneDeep(doc);

I'm fine to roll back the change. In this case, we will pay the penalty of deep cloning all the SO several times. Plus additional work for GC.

We are risking that any step might call stateP.logs.push()

@afharo Can stateP.logs.push be called with the current implementation? I think so. stateP.logs is not an immutable array, State is not frozen object (not sure why, btw)

I'm not quite sure cloneDeep usage is justified here.

After a quick look, I don't think either. Also we're supposed to have unit tests covering these things, so I think we're fine keeping the change.

mshustov · 2021-05-11T12:34:03Z

src/core/server/saved_objects/migrationsv2/types.ts

@@ -370,7 +370,7 @@ export interface LegacyDeleteState extends LegacyBaseState {
  readonly controlState: 'LEGACY_DELETE';
 }

-export type State =
+export type State = Readonly<


note: It's not RecursiveReadonly<...> but we can add it to make sure State is not mutated

mshustov · 2021-05-11T12:56:59Z

src/plugins/dashboard/common/saved_dashboard_references.ts

@@ -24,7 +23,7 @@ export interface SavedObjectAttributesAndReferences {
 }

 const isPre730Panel = (panel: Record<string, string>): boolean => {
-  return 'version' in panel ? semverSatisfies(panel.version, '<7.3') : true;
+  return 'version' in panel ? Semver.gt('7.3.0', panel.version) : true;


noticed that migration spent about 7.4sec here, with these changes, migration is 0.3sec. There are other semverSatisfies usages in dashboard plugin, so it's up to code-owners whether to refactor all the remaining places.

before:

after:

Not surprising given that <7.3 needed to be parsed and interpreted.

ThomThomson

Presentation team changes LGTM - Code only review

afharo

LGTM

mshustov · 2021-05-14T09:59:01Z

@elasticmachine merge upstream

mshustov · 2021-05-17T07:54:18Z

@elasticmachine merge upstream

kibanamachine · 2021-05-17T09:46:51Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	190	217	+27

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	138.7KB	138.7KB	+1.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dashboard`	313.1KB	324.9KB	+11.8KB

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`infra`	261	149	-112
`lens`	67	45	-22
`licensing`	18	15	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`stackAlerts`	101	95	-6
total			-295

History

💔 Build #125969 failed f467e5d
💚 Build #125304 succeeded 2b618f3
💔 Build #125269 failed 2f6a3ce

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Do not clone state, use TypeCheck it's not mutated * do not recreate context for every migration * use more optional semver check * update SavedObjectMigrationContext type * add a test model returns new state object * update docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Do not clone state, use TypeCheck it's not mutated * do not recreate context for every migration * use more optional semver check * update SavedObjectMigrationContext type * add a test model returns new state object * update docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Do not clone state, use TypeCheck it's not mutated * do not recreate context for every migration * use more optional semver check * update SavedObjectMigrationContext type * add a test model returns new state object * update docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

mshustov added 5 commits May 6, 2021 18:41

Do not clone state, use TypeCheck it's not mutated

7d12de8

do not recreate context for every migration

af0ab44

use more optional semver check

c393e54

update SavedObjectMigrationContext type

7aec820

Merge branch 'master' into improve-migration-perf

2f6a3ce

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels May 11, 2021

mshustov requested review from a team as code owners May 11, 2021 10:52

mshustov marked this pull request as draft May 11, 2021 10:52

pgayvallet reviewed May 11, 2021

View reviewed changes

mshustov commented May 11, 2021

View reviewed changes

mshustov added 2 commits May 11, 2021 15:27

add a test model returns new state object

d927702

update docs

2b618f3

mshustov marked this pull request as ready for review May 11, 2021 17:23

ThomThomson approved these changes May 12, 2021

View reviewed changes

afharo approved these changes May 13, 2021

View reviewed changes

Merge branch 'master' into improve-migration-perf

f467e5d

mshustov requested a review from pgayvallet May 14, 2021 09:59

pgayvallet approved these changes May 14, 2021

View reviewed changes

Merge branch 'master' into improve-migration-perf

128fed2

mshustov enabled auto-merge (squash) May 17, 2021 07:54

mshustov merged commit d8a2f8f into elastic:master May 17, 2021

mshustov deleted the improve-migration-perf branch May 17, 2021 12:52

mshustov mentioned this pull request May 18, 2021

[7.x] Improve migration perf (#99773) #100252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve migration perf #99773

Improve migration perf #99773

mshustov commented May 11, 2021 •

edited

Loading

elasticmachine commented May 11, 2021

pgayvallet May 11, 2021

afharo May 11, 2021

mshustov May 11, 2021 •

edited

Loading

pgayvallet May 11, 2021

mshustov May 11, 2021

mshustov May 11, 2021 •

edited

Loading

pgayvallet May 11, 2021

ThomThomson left a comment

afharo left a comment

mshustov commented May 14, 2021

mshustov commented May 17, 2021

kibanamachine commented May 17, 2021

References to deprecated APIs

		let stateP: State = cloneDeep(currentState);
		let stateP: State = currentState;

Improve migration perf #99773

Improve migration perf #99773

Conversation

mshustov commented May 11, 2021 • edited Loading

Summary

Checklist

elasticmachine commented May 11, 2021

pgayvallet May 11, 2021

Choose a reason for hiding this comment

afharo May 11, 2021

Choose a reason for hiding this comment

mshustov May 11, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet May 11, 2021

Choose a reason for hiding this comment

mshustov May 11, 2021

Choose a reason for hiding this comment

mshustov May 11, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet May 11, 2021

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

mshustov commented May 14, 2021

mshustov commented May 17, 2021

kibanamachine commented May 17, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

References to deprecated APIs

History

mshustov commented May 11, 2021 •

edited

Loading

mshustov May 11, 2021 •

edited

Loading

mshustov May 11, 2021 •

edited

Loading