RFR: 8300645: Revert julong changes from 8194232 after 8292083

[Johan Sjölen]

On Mon, 13 Feb 2023 18:13:03 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> The files won't contain negative numbers, AFAIK. Only some random large numbers or the actual limit. `-2` is being returned by the macro if the file doesn't exist or some other error occurs (basically this case). Since this repeatedly causes confusion for me when I revisit this code, I've added the assert for clarity. When we reach line 94, we can be sure that we get a positive value. If you want the assert removed, that's fine with me as well. To me, having the assert there is clearer, though.
>
> I think the `assert(memswlimit > 0, "invariant");` doesn't give a clear explanation why this is the case. And seeing multiple of this kind of assert in the same file is problematic.
> 
> Is it possible for `/memory.limit_in_bytes` to use a number that's larger than `max_jlong`? If so, `GET_CONTAINER_INFO(jlong, ... JLONG_FORMAT)` will fail and this function will return `OSCONTAINER_ERROR`. Previously, this function would return -1.
> 
> (the fact that `OSCONTAINER_ERROR` (-2) can be returned by this function is also problematic).
> 
> I actually think the old code is better. It can be further tightened by changing the blind (and potentially unsafe) cast
> 
> 
>  return (jlong)hier_memswlimit;
> 
> 
> to
> 
> 
>  return as_positive_memory_limit(hier_memswlimit);
> 
> 
> as_positive_memory_limit will assert that the value is not greater than `max_jlong`, which should be guaranteed by the above calculation. More explanations can be added as comments in `as_positive_memory_limit`.

>-2 is being returned by the macro if the file doesn't exist or some other error occurs (basically this case).

Yes, but as you said it'll trigger a short return and never set `memlimit` if it fails, so we'll never see `-2` in `memlimit` regardless.

Now I understand your reasoning, and I get the idea. I do think that if you don't know the macro you'll be confused in one way with the assert, and if you do know the macro you'll be confused without the assert :-). The "real" answer is probably to fix these macros, I think we can do it better. Of course, future RFE.

You read this code more often than I do, I defer to you on keeping or not keeping the `assert`s.  Ping me when you've fixed the log issue please and I'll approve the PR.

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

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