RFR 8222289: Overhaul logic for reading/writing constant pool entries

[Remi Forax]

----- Mail original -----
> De: "Vicente Romero" <vicente.romero at oracle.com>
> À: "compiler-dev" <compiler-dev at openjdk.java.net>
> Envoyé: Mardi 16 Avril 2019 00:04:40
> Objet: Re: RFR 8222289: Overhaul logic for reading/writing constant pool entries

> Hi,
> 
> Very interesting code, kudos for the great job. My main concern was if
> it was possible to get rid of the queue, but after chatting about this
> offline I understood the reasons why you added the queue. In short to
> allow future pool writings to be able to write in any order instead of
> being forced to do a bottom-up writing. The fact that you have hidden
> this process in the very internals of PoolWriter makes easier to write
> new code or fix bugs in this area. Also the queue shouldn't grow too big
> as, even if an entry have dependencies on other entries they don't tend
> to be more than 3.

Apart if you have a condy that reference a condy that reference a condy, etc
A classfile like this will fail with ASM BTW, because we have chosen to use recursive calls instead of a stack/queue.

> Also as you mentioned this code will benefit once we
> have value types in the future, reducing the cost of boxing constants
> into a LoadableConstant. There are a few references to Valhalla in the
> code that, I think, should be removed.
> 
> Thanks,
> Vicente

regards,
Rémi

> 
> On 4/10/19 1:55 PM, Maurizio Cimadamore wrote:
>> Hi,
>> this patch significantly rewrites the way in which constant pool
>> entries are read/written/handled by javac. A good description of the
>> issues in the current architecture is given in the JBS entry [4].
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mcimadamore/8222289/
>>
>> A crucial part of the solution is the new PoolConstant interface. This
>> is an interface that all javac entities that want to be written onto
>> the constant pool must adhere to. The interface has methods for
>> obtaining a pool tag, and, crucially, to obtain a unique key with
>> which the entity can be stored in the pool data structure, so as to
>> quickly check for duplicates.
>>
>> I've also defined a sub-interface of the PoolConstant interface called
>> LoadableConstant, which represents the pool constants that can be
>> loaded with ldc, and stored as static args to bootstrap methods in
>> indy/condy.
>>
>> DynamicMethodSymbol now has been rewritten to express static argument
>> list using LoadableConstant, rather than Object.
>>
>> This patch takes care of most issues in the existing architecture;
>> most notably, thanks to PoolReader, most of the duplication in
>> ModuleNameReader can be removed. Also, I've took great care in
>> removing all eager conversions to pool indices ahead of instruction
>> writing (which then resulted in back-requests to the pool in order to
>> fetch the entry at given address from the Code class). To do this, I
>> tweaked the Code class a bit, so that all methods for emitting opcodes
>> will now take a pool constant object (which can be null if an
>> instruction does not refer to the pool).
>>
>> Some changes were also necessary in Items, more specifically, in order
>> to represent the value of an ImmediateItem as a LoadableConstant,
>> rather than just an Object.
>>
>> The PoolWriter/Reader API is mostly straightforward, with a bunch of
>> putXYZ/getXYZ methods, where XYZ is some Javac entity that needs to be
>> written/read. Among the things to note, PoolWriter needs some special
>> logic to handle the case where a pool entry write request is generated
>> in the middle of another request (a queue has been used to deal with
>> this). The PoolReader uses the same two-phase index + resolution
>> approach used currently by ClassReader (resolving all entries eagerly
>> is not only a performance nightmare, since not all entries will be
>> used, but also a source of bugs, since javac is very sensitive to when
>> entries - esp. those for classes - are turned into real symbols).
>>
>> Extensive testing has been carried out. All relevant tests pass on the
>> new code. In addition to that, I also compared the output of javac
>> before/after the patch and made sure the results were the same. Since
>> the output in general is different (the CP indices before/after are
>> different), I have put together a javap patch [5] which stabilizes the
>> javap output by normalizing all CP indices. This has proven to be a
>> very useful tool, and we might pursue that on a followup change.
>>
>> Performance testing has also been carried out using our internal tools
>> - all results seem to indicate that, at worst, the new code is 1.5%
>> slower than the old one. Given that the new code is significantly more
>> expressive than the old one, I think the performance difference is
>> totally acceptable. Furthermore, when value types will be available I
>> expect a lot of the cost associated with boxing constants (e.g. from
>> String to LoadableConstant) to fade away.
>>
>>
>> Alternatives considered:
>>
>> * Model the constant pool with a proper hierarchy of classes each of
>> which models a different CP entry. This approach has been tried in
>> Valhalla (see [1], [2] and [3]), but ultimately it was rejected
>> because it was not possible to make it perform in a way that was
>> satisfactory. Also, when reading the pool, fully materializing the
>> entries (even the unused ones) was prohibitive, and was avoided (even
>> in the Valhalla branch) in order to have acceptable performances.
>>
>> * Use the new ConstantDesc; while this approach has the potential to
>> get rid of the nasty boxing for primitive constants (thanks to the
>> fact that e.g. String already implements ConstantDesc), the
>> ConstantDesc API is not sufficiently expressive for modeling an entire
>> constant pool. First, not all entries are modeled (e.g. only loadable
>> constant are, by design); secondly, there's no way to query e.g. the
>> pool tag given a ConstantDesc. We might revise the implementation
>> should some of these issues be addressed in future releases.
>>
>> Cheers
>> Maurizio
>>
>>
>> [1] -
>> http://hg.openjdk.java.net/valhalla/valhalla9/langtools/file/85cc92a65da8/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/PoolWriter.java
>> [2] -
>> http://hg.openjdk.java.net/valhalla/valhalla9/langtools/file/85cc92a65da8/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/PoolReader.java
>> [3] -
>> http://hg.openjdk.java.net/valhalla/valhalla9/langtools/file/85cc92a65da8/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Pool.java
>> [4] - https://bugs.openjdk.java.net/browse/JDK-8222289
>> [5] - http://cr.openjdk.java.net/~mcimadamore/x-javapStableOutput_v3/
>>