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

Wed Apr 17 12:57:49 UTC 2019

looks good,
Vicente

On 4/17/19 5:57 AM, Maurizio Cimadamore wrote:
> 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/
>>>
>>>
>>