Skip to content

Cache ObjectMappers to prevent continual recursion during dynamic mapper building #103858

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

Closed
benwtrent wants to merge 3 commits into from
Closed

Cache ObjectMappers to prevent continual recursion during dynamic mapper building #103858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f50a445
Fix dynamic mapping slowness by caching ObjectMapper objects
benwtrent Jan 3, 2024
f739791
Clean up caching for object mapper
benwtrent Jan 3, 2024
ffac8d6
Update docs/changelog/103858.yaml
benwtrent Jan 3, 2024
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
3 changes: 2 additions & 1 deletion ...hmarks/src/main/java/org/elasticsearch/benchmark/index/mapper/DynamicMapperBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ private SourceToParse generateRandomDocument() {
if (random.nextBoolean()) {
continue;
}
String objFieldPrefix = Stream.generate(() -> "obj_field_" + idx).limit(objFieldDepth).collect(Collectors.joining("."));
int objFieldDepthActual = random.nextInt(1, objFieldDepth);
String objFieldPrefix = Stream.generate(() -> "obj_field_" + idx).limit(objFieldDepthActual).collect(Collectors.joining("."));
for (int j = 0; j < textFields; j++) {
if (random.nextBoolean()) {
StringBuilder fieldValueBuilder = generateTextField(fieldValueCountMax);
Expand Down
2 changes: 1 addition & 1 deletion benchmarks/src/main/java/org/elasticsearch/benchmark/index/mapper/MapperServiceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public static MapperService create(String mappings) {
.put("index.number_of_replicas", 0)
.put("index.number_of_shards", 1)
.put(IndexMetadata.SETTING_VERSION_CREATED, IndexVersion.current())
.put("index.mapping.total_fields.limit", 10000)
.put("index.mapping.total_fields.limit", 100000)
.build();
IndexMetadata meta = IndexMetadata.builder("index").settings(settings).build();
IndexSettings indexSettings = new IndexSettings(meta, settings);
Expand Down
6 changes: 6 additions & 0 deletions docs/changelog/103858.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 103858
summary: Cache `ObjectMappers` to prevent continual recursion during dynamic mapper
building
area: Mapping
type: bug
issues: []
7 changes: 4 additions & 3 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ public class DocumentMapper {
* @return the newly created document mapper
*/
public static DocumentMapper createEmpty(MapperService mapperService) {
RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME, ObjectMapper.Defaults.SUBOBJECTS).build(
MapperBuilderContext.root(false, false)
);
RootObjectMapper root = (RootObjectMapper) new RootObjectMapper.Builder(
MapperService.SINGLE_MAPPING_NAME,
ObjectMapper.Defaults.SUBOBJECTS
).build(MapperBuilderContext.root(false, false));
MetadataFieldMapper[] metadata = mapperService.getMetadataMappers().values().toArray(new MetadataFieldMapper[0]);
Mapping mapping = new Mapping(root, metadata, null);
return new DocumentMapper(mapperService.documentParser(), mapping, mapping.toCompressedXContent(), IndexVersion.current());
Expand Down
4 changes: 3 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ static Mapping createDynamicUpdate(DocumentParserContext context) {
for (RuntimeField runtimeField : context.getDynamicRuntimeFields()) {
rootBuilder.addRuntimeField(runtimeField);
}
RootObjectMapper root = rootBuilder.build(MapperBuilderContext.root(context.mappingLookup().isSourceSynthetic(), false));
RootObjectMapper root = (RootObjectMapper) rootBuilder.build(
MapperBuilderContext.root(context.mappingLookup().isSourceSynthetic(), false)
);
return context.mappingLookup().getMapping().mappingUpdate(root);
}

Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/Mapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
public final class Mapping implements ToXContentFragment {

public static final Mapping EMPTY = new Mapping(
new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME, ObjectMapper.Defaults.SUBOBJECTS).build(
(RootObjectMapper) new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME, ObjectMapper.Defaults.SUBOBJECTS).build(
MapperBuilderContext.root(false, false)
),
new MetadataFieldMapper[0],
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ Mapping parse(@Nullable String type, Map<String, Object> mappingSource) throws M
}

return new Mapping(
rootObjectMapper.build(MapperBuilderContext.root(isSourceSynthetic, isDataStream)),
(RootObjectMapper) rootObjectMapper.build(MapperBuilderContext.root(isSourceSynthetic, isDataStream)),
metadataMappers.values().toArray(new MetadataFieldMapper[0]),
meta
);
Expand Down
4 changes: 3 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,19 @@ public Builder(String name, IndexVersion indexCreatedVersion) {
}

Builder includeInRoot(boolean includeInRoot) {
clearObjectMapperCache();
this.includeInRoot = Explicit.explicitBoolean(includeInRoot);
return this;
}

Builder includeInParent(boolean includeInParent) {
clearObjectMapperCache();
this.includeInParent = Explicit.explicitBoolean(includeInParent);
return this;
}

@Override
public NestedObjectMapper build(MapperBuilderContext context) {
protected NestedObjectMapper doBuild(MapperBuilderContext context) {
boolean parentIncludedInRoot = this.includeInRoot.value();
if (context instanceof NestedMapperBuilderContext nc) {
// we're already inside a nested mapper, so adjust our includes
Expand Down
26 changes: 22 additions & 4 deletions server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,25 +81,31 @@ public static class Builder extends Mapper.Builder {
protected Dynamic dynamic;
protected final List<Mapper.Builder> mappersBuilders = new ArrayList<>();
private final Set<String> subMapperNames = new HashSet<>(); // keeps track of dynamically added subfields
// caches the ObjectMapper instance created by build()
// Without the cache, we run into the risk of increased runtime complexity due to the recursive nature of the build() method.
private ObjectMapper objectMapperCache;

public Builder(String name, Explicit<Boolean> subobjects) {
super(name);
this.subobjects = subobjects;
}

public Builder enabled(boolean enabled) {
public final Builder enabled(boolean enabled) {
this.enabled = Explicit.explicitBoolean(enabled);
clearObjectMapperCache();
return this;
}

public Builder dynamic(Dynamic dynamic) {
public final Builder dynamic(Dynamic dynamic) {
this.dynamic = dynamic;
clearObjectMapperCache();
return this;
}

public Builder add(Mapper.Builder builder) {
mappersBuilders.add(builder);
subMapperNames.add(builder.name);
clearObjectMapperCache();
return this;
}

Expand Down Expand Up @@ -174,8 +180,7 @@ protected final Map<String, Mapper> buildMappers(MapperBuilderContext mapperBuil
return mappers;
}

@Override
public ObjectMapper build(MapperBuilderContext context) {
protected ObjectMapper doBuild(MapperBuilderContext context) {
return new ObjectMapper(
name,
context.buildFullName(name),
Expand All @@ -185,6 +190,19 @@ public ObjectMapper build(MapperBuilderContext context) {
buildMappers(context.createChildContext(name))
);
}

@Override
public final ObjectMapper build(MapperBuilderContext context) {
if (objectMapperCache != null) {
return objectMapperCache;
}
objectMapperCache = doBuild(context);
return objectMapperCache;
}

protected void clearObjectMapperCache() {
objectMapperCache = null;
}
Comment on lines +194 to +205
Copy link
Copy Markdown
Member Author

@benwtrent benwtrent Jan 3, 2024

Choose a reason for hiding this comment

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

At this point, I am not sure which is better. Doing this, or reverting: #90674

@javanna ^

Copy link
Copy Markdown
Contributor

@javanna javanna Jan 4, 2024

Choose a reason for hiding this comment

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

Thanks for pinging. I think reverting for now is our wisest option. Happy you have come to a similar conclusion, I think. Long term, we should reconsider what we want to do . Perhaps the updates to the benchmarks is something we do want to get in separately?

Copy link
Copy Markdown
Member Author

@benwtrent benwtrent Jan 4, 2024

Choose a reason for hiding this comment

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

Perhaps the updates to the benchmarks is something we do want to get in separately?

Yes, I will open a separate PR. Its a small adjustment, but showed how we still hadn't addressed the performance issue.

}

public static class TypeParser implements Mapper.TypeParser {
Expand Down
6 changes: 5 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,26 @@ public Builder(String name, Explicit<Boolean> subobjects) {
}

public Builder dynamicDateTimeFormatter(Collection<DateFormatter> dateTimeFormatters) {
clearObjectMapperCache();
this.dynamicDateTimeFormatters = new Explicit<>(dateTimeFormatters.toArray(new DateFormatter[0]), true);
return this;
}

public Builder dynamicTemplates(Collection<DynamicTemplate> templates) {
clearObjectMapperCache();
this.dynamicTemplates = new Explicit<>(templates.toArray(new DynamicTemplate[0]), true);
return this;
}

@Override
public RootObjectMapper.Builder add(Mapper.Builder builder) {
clearObjectMapperCache();
super.add(builder);
return this;
}

public RootObjectMapper.Builder addRuntimeField(RuntimeField runtimeField) {
clearObjectMapperCache();
this.runtimeFields.put(runtimeField.name(), runtimeField);
return this;
}
Expand All @@ -105,7 +109,7 @@ public RootObjectMapper.Builder addRuntimeFields(Map<String, RuntimeField> runti
}

@Override
public RootObjectMapper build(MapperBuilderContext context) {
protected RootObjectMapper doBuild(MapperBuilderContext context) {
return new RootObjectMapper(
name,
enabled,
Expand Down