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

[Vicente Romero]

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. 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

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/
>
>