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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Mon Apr 15 22:36:18 UTC 2019

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 - with the respect to queue I tried 3 approaches:

1) bottom up writing - works, but the PoolWriter::write method has to be 
very careful at doing things in the right order; right now the entries 
are trivial, but when Valhalla will kick in in full, we might start 
generating more complex entries, such as those for parameterized types, 
which might require visitors or other 'complex' means to write them; in 
such cases, I believe it's best if the writer doesn't have to worry 
about the order in which things are written, and just trusts the right 
thing to happen

2) I then experimented with a version of the code that, instead of using 
a queue, used some convoluted logic to update the current entry index, 
so that we could start writing a new entry right in the middle of 
writing another entry (effectively moving the bytecode cursor back and 
forth). While this had no queue, there was no evident performance gain, 
and, at the same time, the code was harder to follow, which led me to 
the current approach. I too am alergic to ordering and queues when I see 
them, but here I think it's matter of compromises.

I'll take a look at the Valhalla reference - it is very possible that 
some of them survived in the transplant :-)


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

More information about the compiler-dev mailing list