Skip to content

Commit 4a5e659

Browse files
l46kokcopybara-github
authored andcommitted
Remove unnecessary synchronization on dispatcher by making it immutable
PiperOrigin-RevId: 827654282
1 parent 4180fc9 commit 4a5e659

File tree

7 files changed

+49
-109
lines changed

7 files changed

+49
-109
lines changed

runtime/src/main/java/dev/cel/runtime/BUILD.bazel

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@ INTERPRABLE_SOURCES = [
6060
# keep sorted
6161
DISPATCHER_SOURCES = [
6262
"DefaultDispatcher.java",
63-
"Dispatcher.java",
6463
]
6564

6665
java_library(
@@ -125,7 +124,6 @@ java_library(
125124
":resolved_overload_internal",
126125
"//:auto_value",
127126
"//common:error_codes",
128-
"//common/annotations",
129127
"//runtime:function_resolver",
130128
"@maven//:com_google_code_findbugs_annotations",
131129
"@maven//:com_google_errorprone_error_prone_annotations",
@@ -146,7 +144,6 @@ cel_android_library(
146144
":resolved_overload_internal_android",
147145
"//:auto_value",
148146
"//common:error_codes",
149-
"//common/annotations",
150147
"@maven//:com_google_code_findbugs_annotations",
151148
"@maven//:com_google_errorprone_error_prone_annotations",
152149
"@maven_android//:com_google_guava_guava",

runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -287,12 +287,12 @@ public CelRuntimeLegacyImpl build() {
287287

288288
functionBindingsBuilder.putAll(customFunctionBindings);
289289

290-
DefaultDispatcher dispatcher = DefaultDispatcher.create();
290+
DefaultDispatcher.Builder dispatcherBuilder = DefaultDispatcher.newBuilder();
291291
functionBindingsBuilder
292292
.buildOrThrow()
293293
.forEach(
294294
(String overloadId, CelFunctionBinding func) ->
295-
dispatcher.add(
295+
dispatcherBuilder.addOverload(
296296
overloadId, func.getArgTypes(), func.isStrict(), func.getDefinition()));
297297

298298
RuntimeTypeProvider runtimeTypeProvider;
@@ -313,7 +313,7 @@ public CelRuntimeLegacyImpl build() {
313313
new DefaultInterpreter(
314314
DescriptorTypeResolver.create(),
315315
runtimeTypeProvider,
316-
dispatcher.immutableCopy(),
316+
dispatcherBuilder.build(),
317317
options);
318318

319319
return new CelRuntimeLegacyImpl(

runtime/src/main/java/dev/cel/runtime/DefaultDispatcher.java

Lines changed: 33 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -14,41 +14,29 @@
1414

1515
package dev.cel.runtime;
1616

17+
import static com.google.common.base.Preconditions.checkNotNull;
18+
19+
import com.google.auto.value.AutoBuilder;
1720
import com.google.common.base.Joiner;
1821
import com.google.common.collect.ImmutableMap;
22+
import com.google.errorprone.annotations.CanIgnoreReturnValue;
1923
import com.google.errorprone.annotations.Immutable;
20-
import javax.annotation.concurrent.ThreadSafe;
21-
import com.google.errorprone.annotations.concurrent.GuardedBy;
2224
import dev.cel.common.CelErrorCode;
2325
import java.util.ArrayList;
24-
import java.util.HashMap;
2526
import java.util.List;
2627
import java.util.Map;
2728
import java.util.Optional;
2829

29-
/**
30-
* Default implementation of {@link Dispatcher}.
31-
*
32-
* <p>Should be final, do not mock; mocking {@link Dispatcher} instead.
33-
*/
34-
@ThreadSafe
35-
final class DefaultDispatcher implements Dispatcher {
36-
public static DefaultDispatcher create() {
37-
return new DefaultDispatcher();
38-
}
30+
/** Default implementation of dispatcher. */
31+
@Immutable
32+
final class DefaultDispatcher implements CelFunctionResolver {
3933

40-
@GuardedBy("this")
41-
private final Map<String, ResolvedOverload> overloads = new HashMap<>();
42-
43-
public synchronized void add(
44-
String overloadId, List<Class<?>> argTypes, boolean isStrict, CelFunctionOverload overload) {
45-
overloads.put(overloadId, CelResolvedOverload.of(overloadId, overload, isStrict, argTypes));
46-
}
34+
private final ImmutableMap<String, ResolvedOverload> overloads;
4735

4836
@Override
49-
public synchronized Optional<ResolvedOverload> findOverload(
37+
public Optional<ResolvedOverload> findOverload(
5038
String functionName, List<String> overloadIds, Object[] args) throws CelEvaluationException {
51-
return DefaultDispatcher.findOverload(functionName, overloadIds, overloads, args);
39+
return findOverload(functionName, overloadIds, overloads, args);
5240
}
5341

5442
/** Finds the overload that matches the given function name, overload IDs, and arguments. */
@@ -87,31 +75,36 @@ public static Optional<ResolvedOverload> findOverload(
8775
return Optional.ofNullable(match);
8876
}
8977

90-
@Override
91-
public synchronized Dispatcher.ImmutableCopy immutableCopy() {
92-
return new ImmutableCopy(overloads);
78+
static Builder newBuilder() {
79+
return new AutoBuilder_DefaultDispatcher_Builder();
9380
}
9481

95-
@Immutable
96-
private static final class ImmutableCopy implements Dispatcher.ImmutableCopy {
97-
private final ImmutableMap<String, ResolvedOverload> overloads;
82+
@AutoBuilder(ofClass = DefaultDispatcher.class)
83+
abstract static class Builder {
9884

99-
private ImmutableCopy(Map<String, ResolvedOverload> overloads) {
100-
this.overloads = ImmutableMap.copyOf(overloads);
101-
}
85+
abstract ImmutableMap<String, ResolvedOverload> overloads();
10286

103-
@Override
104-
public Optional<ResolvedOverload> findOverload(
105-
String functionName, List<String> overloadIds, Object[] args)
106-
throws CelEvaluationException {
107-
return DefaultDispatcher.findOverload(functionName, overloadIds, overloads, args);
108-
}
87+
abstract ImmutableMap.Builder<String, ResolvedOverload> overloadsBuilder();
88+
89+
@CanIgnoreReturnValue
90+
Builder addOverload(
91+
String overloadId,
92+
List<Class<?>> argTypes,
93+
boolean isStrict,
94+
CelFunctionOverload overload) {
95+
checkNotNull(overloadId);
96+
checkNotNull(argTypes);
97+
checkNotNull(overload);
10998

110-
@Override
111-
public Dispatcher.ImmutableCopy immutableCopy() {
99+
overloadsBuilder()
100+
.put(overloadId, CelResolvedOverload.of(overloadId, overload, isStrict, argTypes));
112101
return this;
113102
}
103+
104+
abstract DefaultDispatcher build();
114105
}
115106

116-
private DefaultDispatcher() {}
107+
DefaultDispatcher(ImmutableMap<String, ResolvedOverload> overloads) {
108+
this.overloads = overloads;
109+
}
117110
}

runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ final class DefaultInterpreter implements Interpreter {
6464

6565
private final TypeResolver typeResolver;
6666
private final RuntimeTypeProvider typeProvider;
67-
private final Dispatcher dispatcher;
67+
private final DefaultDispatcher dispatcher;
6868
private final CelOptions celOptions;
6969

7070
/**
@@ -102,7 +102,7 @@ static IntermediateResult create(Object value) {
102102
public DefaultInterpreter(
103103
TypeResolver typeResolver,
104104
RuntimeTypeProvider typeProvider,
105-
Dispatcher dispatcher,
105+
DefaultDispatcher dispatcher,
106106
CelOptions celOptions) {
107107
this.typeResolver = checkNotNull(typeResolver);
108108
this.typeProvider = checkNotNull(typeProvider);
@@ -120,20 +120,20 @@ public Interpretable createInterpretable(CelAbstractSyntaxTree ast) {
120120
static final class DefaultInterpretable implements Interpretable, UnknownTrackingInterpretable {
121121
private final TypeResolver typeResolver;
122122
private final RuntimeTypeProvider typeProvider;
123-
private final Dispatcher.ImmutableCopy dispatcher;
123+
private final DefaultDispatcher dispatcher;
124124
private final Metadata metadata;
125125
private final CelAbstractSyntaxTree ast;
126126
private final CelOptions celOptions;
127127

128128
DefaultInterpretable(
129129
TypeResolver typeResolver,
130130
RuntimeTypeProvider typeProvider,
131-
Dispatcher dispatcher,
131+
DefaultDispatcher dispatcher,
132132
CelAbstractSyntaxTree ast,
133133
CelOptions celOptions) {
134134
this.typeResolver = checkNotNull(typeResolver);
135135
this.typeProvider = checkNotNull(typeProvider);
136-
this.dispatcher = checkNotNull(dispatcher).immutableCopy();
136+
this.dispatcher = checkNotNull(dispatcher);
137137
this.ast = checkNotNull(ast);
138138
this.metadata = new DefaultMetadata(ast);
139139
this.celOptions = checkNotNull(celOptions);

runtime/src/main/java/dev/cel/runtime/Dispatcher.java

Lines changed: 0 additions & 48 deletions
This file was deleted.

runtime/src/main/java/dev/cel/runtime/LiteRuntimeImpl.java

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -176,22 +176,19 @@ public CelLiteRuntime build() {
176176

177177
functionBindingsBuilder.putAll(customFunctionBindings);
178178

179-
DefaultDispatcher dispatcher = DefaultDispatcher.create();
179+
DefaultDispatcher.Builder dispatcherBuilder = DefaultDispatcher.newBuilder();
180180
functionBindingsBuilder
181181
.buildOrThrow()
182182
.forEach(
183183
(String overloadId, CelFunctionBinding func) ->
184-
dispatcher.add(
185-
overloadId,
186-
func.getArgTypes(),
187-
func.isStrict(),
188-
(args) -> func.getDefinition().apply(args)));
184+
dispatcherBuilder.addOverload(
185+
overloadId, func.getArgTypes(), func.isStrict(), func.getDefinition()));
189186

190187
Interpreter interpreter =
191188
new DefaultInterpreter(
192189
TypeResolver.create(),
193190
CelValueRuntimeTypeProvider.newInstance(celValueProvider),
194-
dispatcher,
191+
dispatcherBuilder.build(),
195192
celOptions);
196193

197194
return new LiteRuntimeImpl(

runtime/src/test/java/dev/cel/runtime/DefaultInterpreterTest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,15 @@ public Object adapt(String messageName, Object message) {
7474
}
7575
};
7676
CelAbstractSyntaxTree ast = celCompiler.compile("[1].all(x, [2].all(y, error()))").getAst();
77-
DefaultDispatcher dispatcher = DefaultDispatcher.create();
78-
dispatcher.add(
77+
DefaultDispatcher.Builder dispatcherBuilder = DefaultDispatcher.newBuilder();
78+
dispatcherBuilder.addOverload(
7979
"error",
8080
ImmutableList.of(long.class),
8181
/* isStrict= */ true,
8282
(args) -> new IllegalArgumentException("Always throws"));
8383
DefaultInterpreter defaultInterpreter =
84-
new DefaultInterpreter(new TypeResolver(), emptyProvider, dispatcher, CelOptions.DEFAULT);
84+
new DefaultInterpreter(
85+
new TypeResolver(), emptyProvider, dispatcherBuilder.build(), CelOptions.DEFAULT);
8586
DefaultInterpretable interpretable =
8687
(DefaultInterpretable) defaultInterpreter.createInterpretable(ast);
8788

0 commit comments

Comments
 (0)