RFR: 8160404: RelocationHolder constructors have bugs [v2]

[John R Rose]

On Mon, 12 Dec 2022 17:56:11 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change to construction and copying of the Relocation and
>> RelocationHolder classes, to eliminate some questionable C++ usage.
>> 
>> The array type for RelocationHandle::_relocbuf is changed from void* to char,
>> because using a char array for raw memory is countenanced by the standard,
>> while not so much for an array of void*.  The desired alignment is maintained
>> via a union, since using alignas is not (yet) permitted in HotSpot code.
>> 
>> There is also now a comment discussing the use of _relocbuf in more detail,
>> including some areas of continued sketchiness wrto standard conformance and
>> reliance on implementation dependent behavior. 
>> 
>> No longer use trivial copy and assignment for RelocationHolder, since that
>> isn't technically correct.  The Relocation in the holder is not trivially
>> copyable, since it is polymorphic.  It seemed to work in practice with the
>> supported compilers, but we shouldn't (and don't need to) rely on it.  Instead
>> we have a new virtual function Relocation::copy_into that copies the most
>> derived object into the holder's _relocbuf using placement new.
>> 
>> Eliminated the implict conversion constructor from Relocation to holder that
>> wordwise copied (to possibly beyond the end of) the Relocation into the
>> holder's _relocbuf.  We could have implemented this more carefully with the
>> new approach (using copy_into), but we don't actually need this conversion.
>> The only use of it was actually a wasted copy (in assembler_x86.cpp).
>> 
>> Eliminated the use of placement new syntax via operator new with a holder
>> argument to copy a Resource object into a holder.  This included runtime
>> verification that the size of the object is not greater than the size of
>> _relocbuf; we now do corresponding verification at compile-time.  This also
>> included an incorrect attempt at a runtime check that the Relocation base
>> class would be at the same address as the derived class being constructed; we
>> now perform that check correctly.  We also discuss in a comment the layout
>> assumption being made (that isn't required by the standard but is provided by
>> all supported compilers), and what to do if we encounter a compiler that
>> behaves differently.
>> 
>> Eliminated the idiom of making a default-constructed holder and then
>> overwriting its held relocation with a newly constructed one, using the afore
>> mentioned (and eliminated) operator new.  Instead, RelocationHolder now has a
>> factory function template (construct<T>) for creating holders with a given
>> resource type, constructed using provided arguments.  (The arguments are taken
>> as const-ref rather than using perfect forwarding, as the tools for the latter
>> are not (yet) approved for use in HotSpot. Using const-ref is good enough in
>> this case.)
>> 
>> Describe and verify other assumptions being made, such as all Relocation
>> classes being trivially destructible.
>> 
>> Testing:
>> mach5 tier1-5
>> 
>> Future work:
>> 
>> * RelocationHolder::reloc isn't const-correct.  Making it so will require
>> adjustment of some callers.  I'll follow up with an RFE to address this.
>> 
>> * Relocation classes have many virtual function overrides that are unmarked.
>> I'll follow up with an RFE to add "override" specifiers.
>> 
>> Potential issue: The removal of RelocationHolder(Relocation*) might not work
>> for some platforms.  I've tested on platforms supported by Oracle (where there
>> was only one (mistaken) use).  There might be uses by other platforms.
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
> 
>   blank lines in include blocks

I just saw the query from Dean Long in the bug comments.  Here is some background information.

This "new relocations" work, a long time ago, was an attempt to add some object oriented structure to the access to relocations.  At the time, I didn't trust the resource allocation mechanism, but I wanted a way to decode "real C++ objects" from a relocation stream.  (Being "real" means supporting virtual methods and a strongly typed API that includes such methods.)  The requirement was to be able to iterate quickly over a compressed stream of reloc info and expose the relevant parts as temporary objects to the client.  (This is still a good design pattern; nowadays we have the more successful field stream API, and perhaps there is more to come.)  In order to make iteration over reloc info fast, I decided to allow those temporary objects to unpack themselves (from the compressed reloc info data) into a buffer on the stack rather than on the heap.  The theory is that as you iterate over the stream, you are using just one cache line to access each record, and you get the benefit of a "
 real" object API.  I think sometimes this is called the design pattern of "flyweight objects".

At the time, back in the day, I knew that I was coloring outside the lines of C++, but I knew (or hoped) that C++ compilers were not smart enough to detect my transgression beyond the language.  Those days are gone now.

I am grateful for the current cleanup.  I hope that it can preserve the basic idea of a "flyweight object", which is an object whose storage is inside a fixed cache line associated with a stream, and whose lifetime ends when the stream is advanced, to the next flyweight object in line.  I hope this because I think that compressed streams are here to stay in the JVM, and yet it is desirable to present the contents of such streams as (temporarily unpacked flyweight) C++ objects, real objects.

Here's another peek beyond the horizon, according to me (and this is just me now):  I think that the best representation for JVM metadata is (often, maybe even always) something that is stream-oriented and position independent and pointer-free.  For example, when we boot up a JVM image, if that image has JVM metadata that was "cooked" before startup, we need to quickly warm up that data so it works in the current JVM process.  In such a situation, today's standard C++ data, which is rich in C pointers (which are machine addresses in full machine words that name positions in virtual memory mappings) has a cost; you need to either make sure that the existing pointer values (as seen during CDS generation or Leyden condensation) are still valid, or else you need to edit all such pointer values so they point to the right place in the new address space.  (This is what relocation records do in C++.  Not the same as this PR.)  This means that today's standard C++ data is actually expensive t
 o build ahead of time, before bootstrap. It would be better if the JVM just accessed blocks of data (probably compressed) which use offsets or other non-pointer means to tie themselves together, like today's reloc info or field info streams.

If you buy all that, then there is a permanent place for "flyweight objects" in the JVM, because when you unpack some sort of position-independent data (pointer free data) you probably want to present it to the client as a "real C++ object".  But in many cases, it is OK if that object has a very short lifetime, and correspondingly constrained allocation, to the workflow of the stream abstraction that is stepping through the pointer-free data.

And that is another reason I'm grateful that Kim has read the C++ manual and figured out a way to do the flyweight objects of reloc info within the C++ rules.

-------------

PR: https://git.openjdk.org/jdk/pull/11618