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

Wed Apr 17 09:57:07 UTC 2019

This is  a new revision:

http://cr.openjdk.java.net/~mcimadamore/8222289_v2/webrev/

I had to merge against some changes in the same area. Rerun tests, all 
clean.

I also removed the references to Valhalla.

Maurizio

On 15/04/2019 23:04, Vicente Romero wrote:
> 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/
>>
>>
>