On Tue, 5 Oct 2021 20:39:30 GMT, Leo Korinth <lkori...@openjdk.org> wrote:
>> The basic problem is that we are relying on undefined behaviour, as
>> documented in the code:
>>
>> // This whole business of passing information from ResourceObj::operator new
>> // to the ResourceObj constructor via fields in the "object" is technically
>> UB.
>> // But it seems to work within the limitations of HotSpot usage (such as no
>> // multiple inheritance) with the compilers and compiler options we're using.
>> // And it gives some possibly useful checking for misuse of ResourceObj.
>>
>>
>> I am removing the undefined behaviour by passing the type of allocation
>> through a thread local variable.
>>
>> This solution has some advantages:
>> 1) it is not UB
>> 2) it is simpler and easier to understand
>> 3) it uses less memory (I could make it use even less if I made the enum
>> `allocation_type` a u8)
>> 4) in the *very* unlikely situation that stack memory (or embedded) already
>> equals the data calculated from the address of the object, the code will
>> also work.
>>
>> When doing the change, I also updated `allocated_on_stack()` to the new
>> name `allocated_on_stack_or_embedded()` which is much harder to misinterpret.
>>
>> I also disallow to "fake" the memory type by explicitly calling
>> `ResourceObj::set_allocation_type`.
>>
>> This forced me to change two places that is faking the allocation type of an
>> embedded `GrowableArray` from `STACK_OR_EMBEDDED` to `C_HEAP`. The faking
>> of the type is hard to understand as a `STACK_OR_EMBEDDED` `GrowableArray`
>> can allocate any type of object. My guess is that `GrowableArray` has
>> changed behaviour, or maybe that it was hard to understand because the old
>> naming of `allocated_on_stack()`.
>>
>> I have also tried to update the comments. In doing that I not only changed
>> the comments for this change, but also for the *incorrect* advice to always
>> delete object you allocate with new.
>>
>> Testing on debug build tier1-3
>> Testing on release build tier1
>
> Leo Korinth has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use a thread local buffer so that the compiler might reorder operator new.
Changes requested by kbarrett (Reviewer).
src/hotspot/share/asm/codeBuffer.cpp line 143:
> 141: NOT_PRODUCT(clear_strings());
> 142:
> 143: assert(_default_oop_recorder.allocated_on_stack_or_embedded(), "should
> be embedded object");
It might have been better to do this name change separately, to reduce
distraction.
src/hotspot/share/asm/codeBuffer.hpp line 369:
> 367: // addresses in a sibling section.
> 368:
> 369: class CodeBuffer: public ResourceObj DEBUG_ONLY(COMMA private Scrubber) {
Deriving from ResourceObj rather than the previous fakery seems good. I think
the multiple-inheritance derivation from Scrubber could be removed, instead
adding ~CodeBuffer to do the zapping.
src/hotspot/share/asm/codeBuffer.hpp line 376:
> 374: // CodeBuffers must be allocated on the stack except for a single
> 375: // special case during expansion which is handled internally. This
> 376: // is done to guarantee proper cleanup of resources.
This comment seems dangling now that the associated operator definitions have
been removed.
src/hotspot/share/classfile/stackMapFrame.hpp line 66:
> 64:
> 65: StackMapFrame(const StackMapFrame& cp) :
> 66: ResourceObj(),
Good find.
src/hotspot/share/gc/g1/g1Allocator.hpp line 243:
> 241: _g1h(g1h),
> 242: _allocation_region(NULL),
> 243: _allocated_regions(2, mtGC),
Good riddance.
src/hotspot/share/memory/allocation.cpp line 154:
> 152:
> 153: #ifdef ASSERT
> 154: thread_local ResourceObj::RecentAllocations
> ResourceObj::_recent_allocations;
Don't use `thread_local`. See the style guide.
src/hotspot/share/memory/allocation.cpp line 234:
> 232: }
> 233: #endif // ASSERT
> 234:
Keep the blank line after the `#endif`
src/hotspot/share/memory/allocation.hpp line 398:
> 396: class ResourceObj ALLOCATION_SUPER_CLASS_SPEC {
> 397: public:
> 398: enum allocation_type : uint8_t { STACK_OR_EMBEDDED, RESOURCE_AREA,
> C_HEAP, ARENA };
Consider adding an "empty" allocation-type, that indicates an entry in the
RecentAllocations is unused.
src/hotspot/share/memory/allocation.hpp line 414:
> 412: // allocation is done in a recursive step. In that case an assert will
> trigger.
> 413: class RecentAllocations {
> 414: static const unsigned BufferSize = 5;
"5" seems like an odd choice.
src/hotspot/share/memory/allocation.hpp line 416:
> 414: static const unsigned BufferSize = 5;
> 415: uintptr_t _begin[BufferSize];
> 416: uintptr_t _past_end[BufferSize];
I think these should be `const void*` rather than `uintptr_t`.
src/hotspot/share/memory/memRegion.hpp line 110:
> 108: // A ResourceObj version of MemRegionClosure
> 109:
> 110: class MemRegionClosureRO: public ResourceObj {
I think this class could be deleted entirely, with the only derived class
(`DirtyCardToOopClosure`) instead deriving from ResourceObj.
src/hotspot/share/prims/jvmtiDeferredUpdates.hpp line 132:
> 130: JvmtiDeferredUpdates() :
> 131: _relock_count_after_wait(0),
> 132: _deferred_locals_updates(1, mtCompiler) { }
Good riddance.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5387
