RFR: Small fixes - shipment one

[Coleen Phillimore]

On Tue, 22 Mar 2022 14:18:42 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

> Reducing the number of variables, the number of assignment to each variable, white-spaces, renamed some variables, removed unnecessary template, etc...
> Except the removal of template 'bottom', the code is the same. (assert is moved up one level)
> 
> Passes 1-3 (no new failures)
> It seem to be slightly faster than before, but at least on par.
> 
> This is not complete at all, but to avoid merge conflict I want to ship, and also make someone happy about removing a template.

I like these cleanups.  Only one I had a question about.  Thank you.

src/hotspot/share/runtime/continuation.cpp line 180:

> 178: #define VERIFY_CONTINUATION(cont) verify_continuation((cont))
> 179: NOINLINE static bool verify_stack_chunk(oop chunk) { return InstanceStackChunkKlass::verify(chunk); }
> 180: #define VERIFY_STACK_CHUNK(chunk) verify_stack_chunk((chunk))

Thanks for fixing this.  I know Ron likes to see the asserts have a line number of the caller but there's a PR to fix this and almost all of use some debugger to see where the assert comes from in the call stack, if it's not already in the hs_err file.

src/hotspot/share/runtime/continuation.cpp line 1160:

> 1158:     // properties of the continuation on the stack; all sizes are in words
> 1159:     intptr_t* const stack_top     = top_sp + ContinuationHelper::frame_metadata;
> 1160:     const int       stack_argsize = _cont.argsize();

I like this spelled out in the places it's used.  I wonder if _cont.argsize() is the stacksize of the stack chunk oops.  There are no comments really what it is.
cont_size seems to be the stack size of the entire continuation, ie the sum of all the stack chunk oops?  Dunno.  We're going to need some comments.

src/hotspot/share/runtime/continuation.cpp line 1213:

> 1211: 
> 1212:       chunk = allocate_chunk(cont_size + ContinuationHelper::frame_metadata);
> 1213:       if (UNLIKELY(chunk == nullptr || !_thread->cont_fastpath() || _barriers)) { // OOME/probably humongous

This is just a strange set of conditions.  Why would you allocate_chunk if  _thread->cont_fastpath() is false? or _barriers, whatever that means.  Really don't expect a function like allocate_chunk to have side effects.
Maybe in the latter two conditions, you still want to zero out the chunk in the tail and don't want to remove this.

src/hotspot/share/runtime/continuation.cpp line 1847:

> 1845:     assert((intptr_t)chunk->start_address() % 8 == 0, "");
> 1846: 
> 1847:     init_chunk(chunk);

I tried taking this out as an experiment because I thought after Java heap allocation, it would be already zeroed, but it wasn't.

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

Marked as reviewed by coleenp (Committer).

PR: https://git.openjdk.java.net/loom/pull/114