> 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: review updates ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5387/files - new: https://git.openjdk.java.net/jdk/pull/5387/files/c9d45906..640f2d83 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5387&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5387&range=02-03 Stats: 11 lines in 4 files changed: 0 ins; 7 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5387.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5387/head:pull/5387 PR: https://git.openjdk.java.net/jdk/pull/5387
