8336492: Regression in lambda serialization #20349

mcimadamore · 2024-07-26T10:31:24Z

This PR consolidates the code for dealing with local captures in both Lower and LambdaToMethod. It does so by adding a new tree scanner, namely CaptureScanner, which is then subclassed by Lower.FreeVarCollector and also by the new LambdaToMethod.LambdaCaptureScanner.

The main idea behind the new visitor is that we can compute the set of (most) captured locals w/o relying on complex state from Lower, and make the logic general enough to work on any tree. This can be done by keeping track of all local variable declarations occurring in a given subtree T. Then, if T contains a reference to a local variable that has not been seen, we can assume that this variable is a captured variable. This simple logic works rather well, and doesn't rely on checking e.g. that the accessed variable has the same owner of the method that owns a local class (which then caused other artifacts such as Lower::ownerToCopyFreeVarsFrom, now removed).

The bigger rewrite is the one in LambdaToMethod. That class featured a custom visitor called LambdaAnalyzerPreprocessor (now removed) which maintains a stack of frames encountered during a visit, and tries to establish which references to local variables needs to be captured by the lambda, as well as whether this needs to be referenced by the lambda. Thanks to the new CaptureScanner visitor, all this logic can be achieved in a very small subclass of CaptureScanner, namely LambdaCaptureScanner.

This removes the need to keep track of frames, and other ancillary data structures. LambdaTranslationContext is now significantly simpler, and its most important field is a Map<VarSymbol, VarSymbol> which maps logical lambda symbols to translated ones (in the generated lambda method). We no longer need to maintain different maps for different kinds of captured symbols.

The code for patching identifiers in a lambda expression also became a lot simpler: we just check if we have pending lambda translation context and, if so, we ask if the identifier symbol is contained in the translation map. Otherwise, we leave the identifier as is.

Fixing the issue referenced by this PR is now very simple: inside LambdaCaptureScanner, we just need to make sure that any identifier referring to a captured local field generated by Lower is re-captured, and the rest just works. To make the code more robust I've added a new variable flag to denote synthetic captured fields generated by Lower.

Compatibility

This PR changes the set of fields that are added to local/anonymous classes. Consider the following case:

class Outer {
    void m(int x) {
        class Local extends Serializable {
            void print() {
                System.out.println(x);
            }
        }
    }
}

This used to generate the following bytecode for Local:

class Outer$1Local {
  final int val$x;
    descriptor: I
  final Outer this$0;
    descriptor: LOuter;
  Outer$1Local();
    descriptor: (LOuter;I)V

  void print();
    descriptor: ()V
}

With this patch, we only generate this:

class Outer$1Local implements java.io.Serializable {
  final int val$x;
    descriptor: I
  Outer$1Local();
    descriptor: (LOuter;I)V

  void print();
    descriptor: ()V
}

That is, the field for this$0 is omitted (as that's not used). This affects the serialized form of this local class, as there's now one less field in the serialized form. While this change is desirable (it brings behavior in sync with lambda expressions, and avoiding capture of enclosing instance might be beneficial for GC reachability), we need to make sure this is ok.

Also, it is possible that the order in which local capture fields appear in classes/constructor parameters of local/anon classes changes slightly, due the fact we're using a different visitor.

Another, minor issue in this rewrite is that now generated methods for non-serializable lambdas can have different names from before. In the old code, a shared counter for all lambdas in a class was used. As the new code shares the same logic for checking name clashing as the one used in serializable lambdas, we now increment the suffix after lambda$foo$ if needed. So if two lambdas are defined in two methods foo and bar, we now get lambda$foo$0 and lambda$foo$bar$0 (before, one of them would have had the index set to 1). While this affects some tests, which name we generate for synthetic lambda methods is a private contract, so I didn't think that preserving 100% compatibility was relevant here.

@cushon perhaps you could help assessing the compatibility impact of this change (as you seem to have a codebase which rely on serialization of local classes/lambdas) ?

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8337558 to be approved
Commit message must refer to an issue

Issues

JDK-8336492: Regression in lambda serialization (Bug - P3)
JDK-8337558: Regression in lambda serialization (CSR)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20349/head:pull/20349
$ git checkout pull/20349

Update a local copy of the PR:
$ git checkout pull/20349
$ git pull https://git.openjdk.org/jdk.git pull/20349/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20349

View PR using the GUI difftool:
$ git pr show -t 20349

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20349.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-26T10:31:56Z

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-07-26T10:32:02Z

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8336492: Regression in lambda serialization

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 324 new commits pushed to the master branch:

358ff19: 8339727: Open source several AWT focus tests - series 1
0c36177: 8340089: Simplify SegmentBulkOperations::powerOfProperty
bacd046: 8321010: RISC-V: C2 RoundVF
5709c37: 8340081: Test java/foreign/TestLinker.java failed failed: missing permission java.lang.foreign.native.threshold.power.fill
b88ff9c: 8339849: Enumerate opto and C1 stubs, generate enums, names, fields and generator calls
ae75ca0: 8314842: zgc/genzgc tests ignore vm flags
5e5942a: 8339794: Open source closed choice tests Azure Pipelines continuous integration (without running tests, for now) #1
81ff91e: 8339531: Improve performance of MemorySegment::mismatch
ab9b72c: 8329035: New Data Destination instructions support
7f1dae1: 8339874: Avoid duplicate checking of trailing slash in ZipFile.getZipEntry
... and 314 more: https://git.openjdk.org/jdk/compare/38bd8a36704a962f0ad1052fd2ec150a61663256...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-07-26T10:32:31Z

@mcimadamore The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mcimadamore · 2024-07-26T10:34:27Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

@@ -2337,7 +2243,7 @@ private boolean shouldEmitOuterThis(ClassSymbol sym) {
        // Enclosing instance field is used
        return true;
      }
-      if (rs.isSerializable(sym.type)) {
+      if (sym.owner.kind != MTH && rs.isSerializable(sym.type)) {


Note, this change serialized form for local/anon classes, by omitting capture of enclosing this where possible. (see discussion on serialization compatibility in the PR).

Note: consistency with the lambda case is kind of nice, but if this turns out to be problematic, we can omit this change from this PR, and always capture this$0 in case of a serializable local/anon class.

mcimadamore · 2024-07-26T13:27:38Z

/csr needed

openjdk · 2024-07-26T13:28:33Z

@mcimadamore has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mcimadamore please create a CSR request for issue JDK-8336492 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2024-07-26T13:31:44Z

Webrevs

archiecobbs · 2024-07-26T16:01:36Z

Refactoring Awesomeness Ratio (not counting unit tests) = 1.58. 👍

…tured parameter matches the name of the captured local (skipping any intermediate synthetic local capture in local classes)

cushon · 2024-07-26T17:41:47Z

This looks exciting!

@cushon perhaps you could help assessing the compatibility impact of this change (as you seem to have a codebase which rely on serialization of local classes/lambdas) ?

I'd be happy to. I have started a round of testing at 76a972b, it'll take about a day to have results.

One caveat is that Google's use of serialization generally doesn't rely on persisting the serialized form, and we had disabled the check for serialization in shouldEmitOuterThis so the enclosing instance could be skipped even for serializable classes. That has been fine for us, but I don't know how well that generalizes. There's some previous discussion about the compatibility impact of omitting this references in JDK-8277965 and linked issues.

vicente-romero-oracle

great simplification, some comments for your consideration

vicente-romero-oracle · 2024-07-26T14:52:54Z