Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize saved objects getScopedClient and HTTP API #68221

Merged
merged 5 commits into from
Jun 5, 2020
+17 −11
Merged

Optimize saved objects getScopedClient and HTTP API #68221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5e446a
Create a single repository to be shared by all calls to getScopedClient
rudolf Jun 4, 2020
ef30e36
Cache migrator.getActiveMappings to improve createRepository
rudolf Jun 4, 2020
fa04609
Use typeregistry.getAllTypes instead of getRootPropertiesObjects(mapp…
rudolf Jun 4, 2020
cc98fa6
Don't validate plugin's config every time it's read
rudolf Jun 4, 2020
9ffcf55
Fix saved_objects_mixin
rudolf Jun 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/core/server/plugins/plugin_context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { map } from 'rxjs/operators';
import { map, shareReplay } from 'rxjs/operators';
import { combineLatest } from 'rxjs';
import { CoreContext } from '../core_context';
import { PluginWrapper } from './plugin';
Expand Down Expand Up @@ -107,8 +107,8 @@ export function createPluginInitializerContext(
* @param ConfigClass A class (not an instance of a class) that contains a
* static `schema` that we validate the config at the given `path` against.
*/
create() {
return coreContext.configService.atPath(pluginManifest.configPath);
create<T>() {
return coreContext.configService.atPath<T>(pluginManifest.configPath).pipe(shareReplay(1));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spaces plugin takes a config value for every getScopedClient call (every incoming request). This does config validation for this config path each time.

kibana/x-pack/plugins/spaces/server/spaces_service/spaces_service.ts

Lines 75 to 82 in c5e446a

const getScopedClient = async (request: KibanaRequest) => {
const [coreStart] = await getStartServices();
const internalRepository = await internalRepositoryPromise;
return config$
.pipe(
take(1),
map((config) => {

We could optimize the plugin to not take a new config value every time but only if a new value was emitted, but this felt like a better fix that will probably benefit other places.

},
createIfExists() {
return coreContext.configService.optionalAtPath(pluginManifest.configPath);
Expand Down
6 changes: 5 additions & 1 deletion src/core/server/saved_objects/migrations/kibana/kibana_migrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export class KibanaMigrator {
private readonly status$ = new BehaviorSubject<KibanaMigratorStatus>({
status: 'waiting',
});
private readonly activeMappings: IndexMapping;

/**
* Creates an instance of KibanaMigrator.
Expand All @@ -100,6 +101,9 @@ export class KibanaMigrator {
validateDoc: docValidator(savedObjectValidations || {}),
log: this.log,
});
// Building the active mappings (and associated md5sums) is an expensive
// operation so we cache the result
this.activeMappings = buildActiveMappings(this.mappingProperties);
}

/**
Expand Down Expand Up @@ -172,7 +176,7 @@ export class KibanaMigrator {
*
*/
public getActiveMappings(): IndexMapping {
return buildActiveMappings(this.mappingProperties);
return this.activeMappings;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/saved_objects/service/lib/repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export class SavedObjectsRepository {
injectedConstructor: any = SavedObjectsRepository
): ISavedObjectsRepository {
const mappings = migrator.getActiveMappings();
const allTypes = Object.keys(getRootPropertiesObjects(mappings));
const allTypes = typeRegistry.getAllTypes().map((t) => t.name);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently Object.entries() which gets called from inside getRootPropertiesObjects is very slow.

Pierre already refactored many places where we use this round about getRootPropertiesObjects way to get all the types, but there's still a few remaining.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some getRootPropertiesObjects were kept because of the config 'virtual' type that wasn't properly registered to the registry. But this has been resolved since, so we're safe to remove all the remaining usages.

Very surprised Object.keys is a bottleneck, even if it makes sense that typeRegistry.getAllTypes() is faster as the registry is based on a Map.

Copy link
Contributor

@mshustov mshustov Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it's not Object.keys but getRootPropertiesObjects mutating an object?

kibana/src/core/server/saved_objects/mappings/lib/get_root_properties_objects.ts

Line 52 in 3d39cd5

acc[key] = value;

v8 cannot optimizie this case https://richardartoul.github.io/jekyll/update/2015/04/26/hidden-classes.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I misread and thought it was just a call to getRootProperties

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the line that showed up in the profiler and took almost 5ms:

kibana/src/core/server/saved_objects/mappings/lib/get_root_properties_objects.ts

Line 46 in ea7c78d

return Object.entries(rootProperties).reduce((acc, [key, value]) => {

const serializer = new SavedObjectsSerializer(typeRegistry);
const visibleTypes = allTypes.filter((type) => !typeRegistry.isHidden(type));

Expand Down
3 changes: 1 addition & 2 deletions src/legacy/server/saved_objects/saved_objects_mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,13 @@ import {
importSavedObjectsFromStream,
resolveSavedObjectsImportErrors,
} from '../../../core/server/saved_objects';
import { getRootPropertiesObjects } from '../../../core/server/saved_objects/mappings';
import { convertTypesToLegacySchema } from '../../../core/server/saved_objects/utils';

export function savedObjectsMixin(kbnServer, server) {
const migrator = kbnServer.newPlatform.__internals.kibanaMigrator;
const typeRegistry = kbnServer.newPlatform.start.core.savedObjects.getTypeRegistry();
const mappings = migrator.getActiveMappings();
const allTypes = Object.keys(getRootPropertiesObjects(mappings));
const allTypes = typeRegistry.getAllTypes().map((t) => t.name);
const schema = new SavedObjectsSchema(convertTypesToLegacySchema(typeRegistry.getAllTypes()));
const visibleTypes = allTypes.filter((type) => !schema.isHiddenType(type));

Expand Down
11 changes: 7 additions & 4 deletions x-pack/plugins/spaces/server/spaces_service/spaces_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,18 @@ export class SpacesService {
return spaceId;
};

const internalRepositoryPromise = getStartServices().then(([coreStart]) =>
coreStart.savedObjects.createInternalRepository(['space'])
);

const getScopedClient = async (request: KibanaRequest) => {
const [coreStart] = await getStartServices();
const internalRepository = await internalRepositoryPromise;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createScopedRepository already creates a new repository for every request, so after the other improvements it's probably not that critical that we re-use this repository instead of creating a new one. But since this is on the hot path it felt like it doesn't hurt to keep this minor improvement around.


return config$
.pipe(
take(1),
map((config) => {
const internalRepository = coreStart.savedObjects.createInternalRepository(['space']);

const callWithRequestRepository = coreStart.savedObjects.createScopedRepository(
request,
['space']
Expand All @@ -92,8 +96,7 @@ export class SpacesService {
internalRepository,
request
);
}),
take(1)
})
)
.toPromise();
};
Expand Down