Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers [v2]
On Thu, 9 Jun 2022 16:04:43 GMT, Claes Redestad wrote: >> To take optimal advantage of the pre-existing optimization for repeated >> filters we could split the application of different types of stringifiers. >> >> The resulting difference in order of evaluation is not observable by >> conventional means since all reference type share the same object >> stringifier, and the others are filtering primitives (floats and doubles) >> which have been passed by value already. >> >> This change neutral on many concatenation expression shapes, but for any >> complex expressions with interleaving float/double and reference parameters >> it brings a straightforward reduction in rebinds and underlying LFs >> generated. For example on the >> [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) >> test there's a modest 2% reduction in total classes loaded with this change >> (from 16209 to 15872) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comments based on review feedback Looks good to me. Like Jorne's observation, it'd benefit everyone if this could generate just a single new lambda form multiple filters, not just for repeated filters if feasible. Maybe something to explore more in the future with a tradeoff of complexity. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.org/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers [v2]
> To take optimal advantage of the pre-existing optimization for repeated > filters we could split the application of different types of stringifiers. > > The resulting difference in order of evaluation is not observable by > conventional means since all reference type share the same object > stringifier, and the others are filtering primitives (floats and doubles) > which have been passed by value already. > > This change neutral on many concatenation expression shapes, but for any > complex expressions with interleaving float/double and reference parameters > it brings a straightforward reduction in rebinds and underlying LFs > generated. For example on the > [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) > test there's a modest 2% reduction in total classes loaded with this change > (from 16209 to 15872) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Improve comments based on review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/9082/files - new: https://git.openjdk.org/jdk/pull/9082/files/51c841e8..4b7c696e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9082=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9082=00-01 Stats: 11 lines in 1 file changed: 7 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9082.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9082/head:pull/9082 PR: https://git.openjdk.org/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers
On Wed, 8 Jun 2022 14:44:33 GMT, Jim Laskey wrote: >> To take optimal advantage of the pre-existing optimization for repeated >> filters we could split the application of different types of stringifiers. >> >> The resulting difference in order of evaluation is not observable by >> conventional means since all reference type share the same object >> stringifier, and the others are filtering primitives (floats and doubles) >> which have been passed by value already. >> >> This change neutral on many concatenation expression shapes, but for any >> complex expressions with interleaving float/double and reference parameters >> it brings a straightforward reduction in rebinds and underlying LFs >> generated. For example on the >> [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) >> test there's a modest 2% reduction in total classes loaded with this change >> (from 16209 to 15872) > > I make heavy use of the `mh = filterArgument(baseMH, 0, arrayOfFilters);` > pattern. I assumed it generated fewer LF than making individual calls. Am I > wrong? @JimLaskey no, you're generally right to assume that bunching them up reduces the number of LFs created. There's a flaw in the `makeRepeatedFilterForm` transform that means the theoretical total amount of forms can surpass the baseline if you do _both_ one-by-one and all-at-once transforms from the same root MH. That's a very big if, but it's something I've felt slight unease about since I did that optimization. - PR: https://git.openjdk.java.net/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers
On Wed, 8 Jun 2022 10:20:37 GMT, Claes Redestad wrote: > To take optimal advantage of the pre-existing optimization for repeated > filters we could split the application of different types of stringifiers. > > The resulting difference in order of evaluation is not observable by > conventional means since all reference type share the same object > stringifier, and the others are filtering primitives (floats and doubles) > which have been passed by value already. > > This change neutral on many concatenation expression shapes, but for any > complex expressions with interleaving float/double and reference parameters > it brings a straightforward reduction in rebinds and underlying LFs > generated. For example on the > [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) > test there's a modest 2% reduction in total classes loaded with this change > (from 16209 to 15872) I make heavy use of the `mh = filterArgument(baseMH, 0, arrayOfFilters);` pattern. I assumed it generated fewer LF than making individual calls. Am I wrong? - PR: https://git.openjdk.java.net/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers
On Wed, 8 Jun 2022 13:42:11 GMT, Jorn Vernee wrote: > Looking at the code in MethodHandles::filterArguments, and this optimization. > It seems that for multiple filters we could potentially always generate just > a single new lambda form if we wanted, not just for repeated filters. > > It would be similar to `LambdaFormEditor::makeRepeatedFilterForm`. Just need > to extend the species data with one new `L_TYPE` field for each unique > filter, generate a new getCombiner `Name` for each unique combiner, pass in > an array of combiner types instead of just a single one, and pass in an > additional array of ints that marks the combiner used by each argument > position. Then do similar wiring up as we do now, but calling the > corresponding combiner for each argument pos. Right? > > (If I'm right about that, maybe that's even a more interesting direction to > explore, since it would benefit everyone, and the code in StringConcatFactory > could stay the same) You're right, but I think such a transform would add quite a bit of complexity. Back when I added the `makeRepeatedFilterForm` optimization (for the benefit of `StringConcatFactory` ) I sought to strike a balance between the simple repeatedly-create-`filterArgumentForm`s transform we would do before and more complex forms that would remove more intermediates. I was also a bit concerned that such "shortcutting" transforms could mean we eliminate sharing, and the more complex we make things the harder it gets to reason about. Example: ... mh1 = filterArgument(baseMH, 0, filter); mh1 = filterArgument(mh1, 0, filter); mh2 = filterArgument(baseMH, 0, filter, filter); mh1.form() == mh2.form(); // false The two different pathways end up with two different LFs, since `makeRepeatedFilterForm` shortcuts. In practice this might be a non-issue, but I figured it'd be nice if we could have a reasonable pathway to resolve mh1.form() and mh2.form() to the same thing in the future. - PR: https://git.openjdk.java.net/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers
On Wed, 8 Jun 2022 10:20:37 GMT, Claes Redestad wrote: > To take optimal advantage of the pre-existing optimization for repeated > filters we could split the application of different types of stringifiers. > > The resulting difference in order of evaluation is not observable by > conventional means since all reference type share the same object > stringifier, and the others are filtering primitives (floats and doubles) > which have been passed by value already. > > This change neutral on many concatenation expression shapes, but for any > complex expressions with interleaving float/double and reference parameters > it brings a straightforward reduction in rebinds and underlying LFs > generated. For example on the > [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) > test there's a modest 2% reduction in total classes loaded with this change > (from 16209 to 15872) This looks good to me, but... Looking at the code in MethodHandles::filterArguments, and this optimization. It seems that for multiple filters we could potentially always generate just a single new lambda form if we wanted, not just for repeated filters. It would be similar to `LambdaFormEditor::makeRepeatedFilterForm`. Just need to extend the species data with one new `L_TYPE` field for each unique filter, generate a new getCombiner `Name` for each unique combiner, pass in an array of combiner types instead of just a single one, and pass in an additional array of ints that marks the combiner used by each argument position. Then do similar wiring up as we do now, but calling the corresponding combiner for each argument pos. Right? (If I'm right about that, maybe that's even a more interesting direction to explore, since it would benefit everyone, and the code in StringConcatFactory could stay the same) - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers
On Wed, 8 Jun 2022 12:11:30 GMT, Сергей Цыпанов wrote: >> To take optimal advantage of the pre-existing optimization for repeated >> filters we could split the application of different types of stringifiers. >> >> The resulting difference in order of evaluation is not observable by >> conventional means since all reference type share the same object >> stringifier, and the others are filtering primitives (floats and doubles) >> which have been passed by value already. >> >> This change neutral on many concatenation expression shapes, but for any >> complex expressions with interleaving float/double and reference parameters >> it brings a straightforward reduction in rebinds and underlying LFs >> generated. For example on the >> [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) >> test there's a modest 2% reduction in total classes loaded with this change >> (from 16209 to 15872) > > src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line > 509: > >> 507: doubleFilters = new MethodHandle[ptypes.length]; >> 508: } >> 509: doubleFilters[i] = doubleStringifier(); > > Why do we have `ptypes[i] = int.class` for `int` and `short`, `ptypes[i] = > String.class` for `float`, `double` and `Object` and none for `boolean` and > `char`? 類 `Object`, `float` and `double` arguments are all eagerly transformed to `String` (this is what the "stringifiers" do). Since we apply these as filters as a last step (which means that's the first thing that will run) we then must change the internal type to `String`. Ultimately means we have fewer types to worry about in the combinator. We also widen integral subword types to `int` (`short` and `byte`). This doesn't need to be adapted by filtering but can lean on implicit conversion (subword primitives are widened to int anyhow, what we care about is matching the correct logic). This mainly matters insofar that the same direct method handle will be used for these three types, as it can (and will) be adapted without modification by the `viewAsType` call that ensures the `MH` returned from the BSM has the exact type asked for. `boolean` and `char` types can't use the `int` mixers/prependers and instead have specialized methods that will be adapted into the concat MH tree. This means the parameter type stays `boolean` and `char` throughout, respectively. - PR: https://git.openjdk.java.net/jdk/pull/9082
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers
On Wed, 8 Jun 2022 10:20:37 GMT, Claes Redestad wrote: > To take optimal advantage of the pre-existing optimization for repeated > filters we could split the application of different types of stringifiers. > > The resulting difference in order of evaluation is not observable by > conventional means since all reference type share the same object > stringifier, and the others are filtering primitives (floats and doubles) > which have been passed by value already. > > This change neutral on many concatenation expression shapes, but for any > complex expressions with interleaving float/double and reference parameters > it brings a straightforward reduction in rebinds and underlying LFs > generated. For example on the > [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) > test there's a modest 2% reduction in total classes loaded with this change > (from 16209 to 15872) src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 509: > 507: doubleFilters = new MethodHandle[ptypes.length]; > 508: } > 509: doubleFilters[i] = doubleStringifier(); Why do we have `ptypes[i] = int.class` for `int` and `short`, `ptypes[i] = String.class` for `float`, `double` and `Object` and none for `boolean` and `char`? 類 - PR: https://git.openjdk.java.net/jdk/pull/9082
