Skip to content

Memoize JsInliner.containsNestedFunctions #10397

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

Open
rhuanhianc wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Memoize JsInliner.containsNestedFunctions #10397

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58e1c90
Memoize JsInliner.containsNestedFunctions
rhuanhianc Aug 19, 2026
8e6f11b
Use computeIfAbsent for nested function cache
rhuanhianc Aug 19, 2026
958773b
Drop redundant nested function cache invalidation
rhuanhianc Sep 2, 2026
ec9f0b4
Scope the nested function cache to the inlining pass
rhuanhianc Sep 2, 2026
e8e53ac
Drop the post-inline nested function cache invalidation
rhuanhianc Sep 2, 2026
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
21 changes: 20 additions & 1 deletion dev/core/src/com/google/gwt/dev/js/JsInliner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@

import java.util.Collection;
import java.util.Collections;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
Expand Down Expand Up @@ -802,6 +803,8 @@ public void endVisit(JsInvocation x, JsContext ctx) {
*/
op = accept(op);
ctx.replaceMe(op);
// The accept above may have re-cached the caller while op was still detached.
containsNestedFunctionsCache.get().remove(callerFunction);

@niloc132 niloc132 Sep 2, 2026

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.

I'm not quite understanding this remark - this seems to imply that there's a risk of a function being added or removed during process? Probably would be worth temporarily reverting this, writing a (failing) test, and confirming that the test now passes?

@rhuanhianc rhuanhianc Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried to write that failing test like you suggested and couldn't, and after digging into why, my note there was just wrong. The case can't happen at all.

convertToExpression clones every callee statement through JsSafeCloner, and that returns null for anything holding a JsFunction. So a function with a nested function inside is never inlinable in the first place, and the body that replaces the invocation never carries a function into the caller. The only other thing that moves in is the caller's own arguments, which were already there — and an argument holding a function is always volatile (AffectedBySideEffectsVisitor.endVisit(JsFunction)), so it goes into requiredOrder, and EvaluationOrderVisitor.maintainsOrder() needs it referenced in the inlined body, so it can't get dropped either.

The answer can only go true → false, and that's the safe direction.

Rather than just delete the line and leave the reasoning in a comment, I made containsNestedFunctions recompute and compare on every cache hit under an assert. The dev tests run with -ea, so now it's checked on every lookup of every test instead of by one contrived case.

I did check it before removing anything, too. An instrumented build that recomputed and compared on every lookup, with the invalidation disabled, reported 6,683 cache hits and zero mismatches compiling one application, and a full run over a large one with the assert on passes as well — the inliner finishes well before JavaScriptVerifier runs.

And the two tests you asked for are in. Both answers of the predicate show up as a difference in the generated output, so forcing it to a constant breaks one of them.

}

if (inlining.pop() != invokedFunction) {
Expand Down Expand Up @@ -1021,6 +1024,8 @@ && isInvokedMoreThanOnce(invokedFunction)) {
return x;
}

containsNestedFunctionsCache.get().remove(callerFunction);

@niloc132 niloc132 Sep 2, 2026

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.

Without a comment (like the other remove() func has), it seems like this is the same basic idea - invalidate the cache since we're changing the function in question.

However, this is before the function has been modified - the other site is just after the replaceMe(), so it seems that if accept() might re-add items, we shouldn't bother with this line at all?

@rhuanhianc rhuanhianc Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's right, and it's easy to check: past that line process has no return x left, and the expression it builds is never the same object as x, so the if (x != op) branch in endVisit always ran and invalidated the caller anyway. Nothing in between touches the caller's body either, since ctx.replaceMe hasn't happened yet, so recomputing there gave back the same value. It was a pure re-traversal. Removed.

The other one is gone too, for the reason in the other thread.


// We've committed to the inlining, ensure the vars are created
newLocalVariableStack.peek().addAll(extrudedNames);

Expand Down Expand Up @@ -1549,6 +1554,13 @@ public boolean visit(JsObjectLiteral x, JsContext ctx) {
private static final int INLINING_BIAS = Integer.parseInt(System.getProperty(
"gwt.jsinlinerInliningBias", "5"));

/**
* Caches {@link #containsNestedFunctions(JsFunction)}, which would otherwise re-traverse a whole
* function body at every call site. Thread local because permutations compile concurrently.
*/
private static final ThreadLocal<Map<JsFunction, Boolean>> containsNestedFunctionsCache =

@niloc132 niloc132 Sep 2, 2026

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.

Wouldn't an instance variable make more sense? This will end up sticking around as long as the thread itself, even if the inliner isn't invoked again.

Granted, this would require other changes - isInlinable, isVolatile, and affectedBySideEffects will need to stop being static (or pass it in as a parameter), but I'm not really sure thats a bad thing - an instance already exists, and these are never called outside the context of having an instance as far as I can tell.

This could also be achieved by moving the clear() to the end of execImpl instead of the start, but I'd mildly prefer the instance var and avoid any concerns about threading.

@rhuanhianc rhuanhianc Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @niloc132 , done. The cache is a field on InliningVisitor now, and isInlinable, isVolatile, affectedBySideEffects and containsNestedFunctions came along with it as instance methods, bodies untouched.

You were right that nothing outside needs them static: process is the only entry into that chain and it has a single call site, so dropping static cost nothing. The ThreadLocal and the clear() in execImpl are both gone, and with them the retention you pointed at.

ThreadLocal.withInitial(IdentityHashMap::new);

/**
* Static entry point used by JavaToJavaScriptCompiler.
*/
Expand Down Expand Up @@ -1589,12 +1601,19 @@ private static int complexity(JsNode toEstimate) {
* Examine a JsFunction to determine if it contains nested functions.
*/
private static boolean containsNestedFunctions(JsFunction func) {
Boolean cached = containsNestedFunctionsCache.get().get(func);
Comment thread
zbynek marked this conversation as resolved.
Outdated
if (cached != null) {
return cached;
}
NestedFunctionVisitor v = new NestedFunctionVisitor();
v.accept(func.getBody());
return v.containsNestedFunctions();
boolean result = v.containsNestedFunctions();
containsNestedFunctionsCache.get().put(func, result);
return result;
}

private static int execImpl(JsProgram program, Collection<JsNode> toInline) {
containsNestedFunctionsCache.get().clear();
try (OptimizerStats stats = OptimizerStats.optimization(NAME)) {

// We are not covering the whole AST, hence we will try to inline functions with a single call
Expand Down