[lworld] RFR: 8251046: [lworld] [lw3] C1 should avoid heap allocations in withfield when possible

[Tobias Hartmann]

On Wed, 5 Aug 2020 13:14:12 GMT, Frederic Parain <fparain at openjdk.org> wrote:

> Please review this patch which reduces the number of heap allocations when executing the withfield bytecode with C1.
> The CR linked below contains a more detailed description of the optimization:
> 
> CR: https://bugs.openjdk.java.net/browse/JDK-8251046
> 
> A rudimentary benchmark shows encouraging numbers:
> Measuring inline type creation time with -XX:TieredStopAtLevel=1
> Inline class Point has two int fields.
> Inline class Rectangle has four int fields.
> 
> Without the optimization:
> Benchmark                                Mode  Samples   Score  Score error  Units
> o.s.MyBenchmark.testPointCreation        avgt      200   7.573        0.027  ns/op
> o.s.MyBenchmark.testRectangleCreation    avgt      200  21.387        0.067  ns/op
> 
> With the optimization:
> Benchmark                                Mode  Samples  Score  Score error  Units
> o.s.MyBenchmark.testPointCreation        avgt      200  5.284        0.014  ns/op
> o.s.MyBenchmark.testRectangleCreation    avgt      200  7.750        0.082  ns/op
> 
> Tested manually and with Mach5, tiers 1 to 3 (with Tobias' changes enabling more C1 testing).
> 
> Thank you,
> 
> Fred

Overall looks good to me, nice improvement! I've added some comments to the code.

src/hotspot/share/c1/c1_Instruction.cpp line 422:

> 421:
> 422:   StoreField::StoreField(Value obj, int offset, ciField* field, Value value, bool is_static,
> 423:              ValueStack* state_before, bool needs_patching)

Indentation should be fixed.

src/hotspot/share/c1/c1_Instruction.cpp line 438:

> 437:
> 438:   StoreIndexed::StoreIndexed(Value array, Value index, Value length, BasicType elt_type, Value value, ValueStack*
> state_before, 439:                bool check_boolean, bool mismatched)

Indentation should be fixed.

src/hotspot/share/c1/c1_GraphBuilder.cpp line 1107:

> 1106:       { Value w = state()->raw_pop();
> 1107:         NewInlineTypeInstance::update_stack_count(w);
> 1108:       }

Wouldn't it be cleaner to use virtual calls on the Value class here?

src/hotspot/share/c1/c1_Instruction.hpp line 1419:

> 1418:   virtual void set_not_larva_anymore() {
> 1419:     _in_larval_state = false; }
> 1420:

No newline needed after "{" or add newline before "}" as well.

-------------

Changes requested by thartmann (Committer).

PR: https://git.openjdk.java.net/valhalla/pull/137