RFR: 8293972: runtime/NMT/NMTInitializationTest.java#default_long-off failed with "Suspiciously long bucket chains in lookup table." [v4]

[Thomas Stuefe]

On Thu, 20 Jul 2023 17:41:24 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

> I really do not like the complexity of `NMTPreInit`

Yeah, I noticed. 

IMHO There are tons of better targets for cleanups and reviewer cycles. Just from the top of my head the NMT backend, the virtual memory abstraction layer, UL, ReservedSpace, Hotspot arena handling... all these are areas are quite complex and often very messy and would benefit a lot from cleanup attention.

NMTPreinit was a large improvement over the way things were. It has made little problems over the years, barring over-strict tests like this one (my own darn fault for writing them, ironically - untested code does not draw attention).

> So I can add `realloc` back to `NMTPreInit` if required, after I consider this issue more and am unable to accommodate your useå cases.
> 
> What about the other change, where we move `NMT` initialization before we process the arguments, which makes the `NMTPreInit` table **much** smaller? Is that part OK?

I like the change to a Mersenne prime, that's a good idea.

But the initialization change not so much. It adds a lot of complexity and brittleness to NMT initialization, for not much benefit.

Right now, NMT initialization is *simple*. Because it is time-independent. All VM subsystems are already started, and we are in normal VM land. We can log, we can use Thread::current, we can use ostream, we can crash and have useful hs_err files... That was a big simplification that NMTPreInit brought. 

With your patch, we are back at having two NMT initialization phases. And the early phase happens before UL or Thread::current() or ostream or reasonable hs_err file printing are available.

And we don't get that much for it. `NMTInitializationTest.java` is a very artificial test, not sure we should optimize for that. A normal debug VM started without arguments has 331 pre-init malloc calls. That drops to 47 with your patch. So, with arguments, I convert maybe 300..400 preinit mallocs to normal os::malloc calls.

I think instead our time would be well spent on tracking down those mallocs and removing them. IIRC A lot of that stuff comes from UL. And UL would benefit *so much* from simplification. It is so darn complex for a simple logging framework. I don't see why it needs to dynamically allocate at all before argument parsing, since it has no way of knowing yet what's what. I suspect whatever is generated there could just as well live as automatic variable, or inlined, or hard-coded as constants.

For this test, I think you could make the failure threshold much higher. When I originally wrote the test, I intended to weed out stupid problems like "oh, hashcode is constant", or "oh, we only use every 16th table bucket because of ptr alignment". But in this case, the hash distribution is not that bad.

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

PR Comment: https://git.openjdk.org/jdk/pull/14607#issuecomment-1645661876