Skip to content

Commit 64b60dd

Browse files
lauraharkercopybara-github
authored andcommitted
Reduce number of full AST traversals in J2clPass from 5 -> 2
FunctionDefsCollector already avoids unnecessary full AST traversals in its "shouldTraverse" method. (It does run a full AST traversal once, for `inlineFunctionsInFile(root, ALL_CLASS_FILE_NAMES`). However, StaticCallInliner traverses the entire AST each time it runs, which is currently 4 times. This CL merges all the StaticCallInliner traversals into one traversal. So we now only have one FunctionDefsCollector traversal & one StaticCallInliner traversal. PiperOrigin-RevId: 851358015
1 parent fd7a8b1 commit 64b60dd

File tree

1 file changed

+91
-58
lines changed

1 file changed

+91
-58
lines changed

src/com/google/javascript/jscomp/J2clPass.java

Lines changed: 91 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,18 @@
1515
*/
1616
package com.google.javascript.jscomp;
1717

18+
import static com.google.common.base.Preconditions.checkState;
19+
import static com.google.common.collect.ImmutableSet.toImmutableSet;
20+
21+
import com.google.common.collect.ImmutableList;
1822
import com.google.common.collect.ImmutableSet;
1923
import com.google.javascript.jscomp.FunctionInjector.InliningMode;
2024
import com.google.javascript.jscomp.FunctionInjector.Reference;
2125
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
2226
import com.google.javascript.rhino.Node;
27+
import java.util.ArrayList;
2328
import java.util.LinkedHashMap;
29+
import java.util.List;
2430
import java.util.Map;
2531
import java.util.Set;
2632
import java.util.function.Supplier;
@@ -42,22 +48,25 @@ public void process(Node externs, Node root) {
4248
return;
4349
}
4450

51+
List<InlineSpec> inlinings = new ArrayList<>();
52+
4553
/*
4654
* Inline functions in Arrays that take references to static $isInstance() functions. This
4755
* ensures that the references will be fully qualified to work with collapse properties.
4856
*/
49-
inlineFunctionsInFile(
50-
root,
51-
"Arrays.impl.java.js",
52-
ImmutableSet.of(
53-
"$create",
54-
"$createWithInitializer",
55-
"$init",
56-
"$instanceIsOfType",
57-
"$castTo",
58-
"$stampType"),
59-
InliningMode.DIRECT);
60-
inlineFunctionsInFile(root, "Casts.impl.java.js", ImmutableSet.of("$to"), InliningMode.DIRECT);
57+
inlinings.add(
58+
new InlineSpec(
59+
"Arrays.impl.java.js",
60+
ImmutableSet.of(
61+
"$create",
62+
"$createWithInitializer",
63+
"$init",
64+
"$instanceIsOfType",
65+
"$castTo",
66+
"$stampType"),
67+
InliningMode.DIRECT));
68+
inlinings.add(
69+
new InlineSpec("Casts.impl.java.js", ImmutableSet.of("$to"), InliningMode.DIRECT));
6170

6271
/*
6372
* Inlines all Interface.$markImplementor(FooClass) metaclass calls so that FooClass and others
@@ -68,27 +77,28 @@ public void process(Node externs, Node root) {
6877
* implementing Java interfaces (not recommended but widely used in xplat) needs calls to
6978
* $markImplementor.
7079
*/
71-
inlineFunctionsInFile(
72-
root, ALL_CLASS_FILE_NAMES, ImmutableSet.of("$markImplementor"), InliningMode.BLOCK);
80+
inlinings.add(
81+
new InlineSpec(
82+
ALL_CLASS_FILE_NAMES, ImmutableSet.of("$markImplementor"), InliningMode.BLOCK));
7383

7484
/*
7585
* Inlines class metadata calls so they become optimizable and avoids escaping of constructor.
7686
*/
77-
inlineFunctionsInFile(
78-
root,
79-
"Util.impl.java.js",
80-
ImmutableSet.of(
81-
"$setClassMetadata",
82-
"$setClassMetadataForInterface",
83-
"$setClassMetadataForEnum",
84-
"$setClassMetadataForPrimitive"),
85-
InliningMode.BLOCK);
87+
inlinings.add(
88+
new InlineSpec(
89+
"Util.impl.java.js",
90+
ImmutableSet.of(
91+
"$setClassMetadata",
92+
"$setClassMetadataForInterface",
93+
"$setClassMetadataForEnum",
94+
"$setClassMetadataForPrimitive"),
95+
InliningMode.BLOCK));
96+
97+
new ClassStaticFunctionsInliner(root, inlinings).run();
8698
}
8799

88-
private void inlineFunctionsInFile(
89-
Node root, String classFileName, Set<String> fnNamesToInline, InliningMode inliningMode) {
90-
new ClassStaticFunctionsInliner(root, classFileName, fnNamesToInline, inliningMode).run();
91-
}
100+
private record InlineSpec(
101+
String classFileName, Set<String> fnNamesToInline, InliningMode inliningMode) {}
92102

93103
/**
94104
* Collects references to certain function definitions in a certain class and then inlines fully
@@ -98,49 +108,60 @@ private void inlineFunctionsInFile(
98108
* collected fully qualified function names once the module prefix has been added.
99109
*/
100110
private class ClassStaticFunctionsInliner {
101-
private final String classFileName;
102-
private final Set<String> fnNamesToInline;
103-
private final InliningMode inliningMode;
104-
private final Map<String, Node> fnsToInlineByQualifiedName = new LinkedHashMap<>();
111+
private final Map<String, InlinableFunction> fnsToInlineByQualifiedName = new LinkedHashMap<>();
105112
private final FunctionInjector injector;
106113
private final Node root;
114+
private final List<InlineSpec> inlineSpecs;
107115

108-
private ClassStaticFunctionsInliner(
109-
Node root, String classFileName, Set<String> fnNamesToInline, InliningMode inliningMode) {
110-
this.root = root;
111-
this.classFileName = classFileName;
112-
this.fnNamesToInline = fnNamesToInline;
113-
this.inliningMode = inliningMode;
116+
private record InlinableFunction(Node impl, InliningMode inliningMode) {}
114117

118+
private ClassStaticFunctionsInliner(Node root, List<InlineSpec> inlineSpecs) {
119+
this.root = root;
120+
this.inlineSpecs = inlineSpecs;
115121
this.injector =
116122
new FunctionInjector.Builder(compiler)
117123
.safeNameIdSupplier(safeNameIdSupplier)
118124
.assumeStrictThis(true)
119125
.assumeMinimumCapture(true)
120126
.build();
121-
this.injector.setKnownConstantFunctions(ImmutableSet.copyOf(fnNamesToInline));
127+
ImmutableSet<String> fnNamesToInline =
128+
inlineSpecs.stream().flatMap(b -> b.fnNamesToInline.stream()).collect(toImmutableSet());
129+
this.injector.setKnownConstantFunctions(fnNamesToInline);
122130
}
123131

124132
private void run() {
125133
NodeTraversal.traverse(compiler, root, new FunctionDefsCollector());
126134
NodeTraversal.traverse(compiler, root, new StaticCallInliner());
127135
}
128136

137+
private ImmutableList<InlineSpec> findMatchingSpecsForFile(String sourceFileName) {
138+
ImmutableList.Builder<InlineSpec> matchingSpecs = ImmutableList.builder();
139+
for (InlineSpec inlineSpec : inlineSpecs) {
140+
if (inlineSpec.classFileName.equals(ALL_CLASS_FILE_NAMES)
141+
|| sourceFileName.endsWith(inlineSpec.classFileName)) {
142+
matchingSpecs.add(inlineSpec);
143+
}
144+
}
145+
return matchingSpecs.build();
146+
}
147+
129148
private class FunctionDefsCollector implements NodeTraversal.Callback {
149+
// This list is initialized every time the node traversal enters a new script. It contains
150+
// all InlineSpecs whose classFileName matches the script's filename.
151+
private ImmutableList<InlineSpec> currFileInlineSpecs;
152+
130153
@Override
131-
public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) {
132-
// Only look inside the referenced class file.
133-
return !n.isScript()
134-
|| n.getSourceFileName().endsWith(classFileName)
135-
|| classFileName.equals(ALL_CLASS_FILE_NAMES);
154+
public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
155+
if (n.isScript()) {
156+
this.currFileInlineSpecs = findMatchingSpecsForFile(n.getSourceFileName());
157+
return !this.currFileInlineSpecs.isEmpty();
158+
}
159+
return true;
136160
}
137161

138162
@Override
139163
public void visit(NodeTraversal t, Node n, Node parent) {
140-
// If we arrive here then we're already inside the desired script.
141-
142164
// Only look at named function declarations that are fully qualified.
143-
final String qualifiedFnName;
144165
final String fnName;
145166
switch (n.getToken()) {
146167
case ASSIGN -> {
@@ -153,22 +174,32 @@ public void visit(NodeTraversal t, Node n, Node parent) {
153174
if (!qualifiedNameNode.isGetProp() || !qualifiedNameNode.isQualifiedName()) {
154175
return;
155176
}
156-
157-
qualifiedFnName = qualifiedNameNode.getQualifiedName();
158177
fnName = qualifiedNameNode.getString();
159178
}
160-
case MEMBER_FUNCTION_DEF -> {
161-
qualifiedFnName = NodeUtil.getBestLValueName(n);
162-
fnName = n.getString();
163-
}
179+
case MEMBER_FUNCTION_DEF -> fnName = n.getString();
164180
default -> {
165181
return;
166182
}
167183
}
168-
169-
if (fnNamesToInline.contains(fnName)) {
184+
for (InlineSpec inlineSpec : currFileInlineSpecs) {
185+
if (!inlineSpec.fnNamesToInline.contains(fnName)) {
186+
continue;
187+
}
170188
// Then store a reference to it.
171-
fnsToInlineByQualifiedName.put(qualifiedFnName, n.getLastChild());
189+
InlinableFunction inlinableFunction =
190+
new InlinableFunction(n.getLastChild(), inlineSpec.inliningMode);
191+
String qualifiedFnName =
192+
switch (n.getToken()) {
193+
case ASSIGN -> n.getFirstChild().getQualifiedName();
194+
case MEMBER_FUNCTION_DEF -> NodeUtil.getBestLValueName(n);
195+
default -> throw new AssertionError();
196+
};
197+
var previousValue = fnsToInlineByQualifiedName.put(qualifiedFnName, inlinableFunction);
198+
checkState(
199+
previousValue == null,
200+
"expected each function to match at most one InliningSpec, but found:\n%s\n%s",
201+
previousValue,
202+
inlinableFunction);
172203
}
173204
}
174205
}
@@ -189,11 +220,13 @@ public void visit(NodeTraversal t, Node n, Node parent) {
189220

190221
// ... and that reference a function definition we want to inline
191222
String qualifiedFnName = qualifiedNameNode.getQualifiedName();
192-
String fnName = qualifiedNameNode.getString();
193-
Node fnImpl = fnsToInlineByQualifiedName.get(qualifiedFnName);
194-
if (fnImpl == null) {
223+
InlinableFunction fn = fnsToInlineByQualifiedName.get(qualifiedFnName);
224+
if (fn == null) {
195225
return;
196226
}
227+
String fnName = qualifiedNameNode.getString();
228+
Node fnImpl = fn.impl();
229+
InliningMode inliningMode = fn.inliningMode();
197230

198231
// Ensure that the function only has a single return statement when direct inlining.
199232
if (inliningMode == InliningMode.DIRECT

0 commit comments

Comments
 (0)