Skip to content

avoiding allocations in FieldCollectorParameters and MergedField #2074

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

Merged
bbakerman merged 2 commits into from
Oct 21, 2020
Merged

avoiding allocations in FieldCollectorParameters and MergedField #2074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae46a40
avoiding LinkedHashMap allocations
dfa1 Oct 15, 2020
ee6ed0d
avoiding ArrayList allocations
dfa1 Oct 15, 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
9 changes: 4 additions & 5 deletions src/main/java/graphql/execution/FieldCollectorParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLSchema;

import java.util.LinkedHashMap;
import java.util.Map;

/**
Expand Down Expand Up @@ -48,8 +47,8 @@ public static Builder newParameters() {

public static class Builder {
private GraphQLSchema graphQLSchema;
private final Map<String, FragmentDefinition> fragmentsByName = new LinkedHashMap<>();
private final Map<String, Object> variables = new LinkedHashMap<>();
private Map<String, FragmentDefinition> fragmentsByName;
private Map<String, Object> variables;
private GraphQLObjectType objectType;

/**
Expand All @@ -70,12 +69,12 @@ public Builder objectType(GraphQLObjectType objectType) {
}

public Builder fragments(Map<String, FragmentDefinition> fragmentsByName) {
this.fragmentsByName.putAll(fragmentsByName);
this.fragmentsByName = fragmentsByName;
return this;
}

public Builder variables(Map<String, Object> variables) {
this.variables.putAll(variables);
this.variables = variables;
return this;

@bbakerman bbakerman Oct 19, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a tricky one - generally we try for more immutable classes and hence by "copying" we gain some control in mutable Java land

However as this class is repeatedly called, we probably need to change this general pattern if we see evidence that it costs us.

@dfa1 dfa1 Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I'm not sure too. I did that because the class is marked as @internal (so we may have more freedom) but I know that it is against the pattern mutable/builder -> immutable/built object.

It looks like it is always called with ExecutionContext data, that is unmodifiable.

}

Expand Down
8 changes: 4 additions & 4 deletions src/main/java/graphql/execution/MergedField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public class MergedField {

private MergedField(List<Field> fields) {
assertNotEmpty(fields);
this.fields = unmodifiableList(new ArrayList<>(fields));
this.fields = unmodifiableList(fields);
this.singleField = fields.get(0);
this.name = singleField.getName();
this.resultKey = singleField.getAlias() != null ? singleField.getAlias() : name;
Expand Down Expand Up @@ -144,7 +144,8 @@ public MergedField transform(Consumer<Builder> builderConsumer) {
}

public static class Builder {
private List<Field> fields;

private final List<Field> fields;

private Builder() {
this.fields = new ArrayList<>();
Expand All @@ -155,7 +156,7 @@ private Builder(MergedField existing) {
}

public Builder fields(List<Field> fields) {
this.fields = fields;
this.fields.addAll(fields);
return this;

@dfa1 dfa1 Oct 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

used only in tests, could be a breaking change.

Maybe this is the root cause of the additional copy that I've just removed in the constructor.

}

Expand All @@ -168,7 +169,6 @@ public MergedField build() {
return new MergedField(fields);
}


}

@Override
Expand Down