RFR: 8296453: Simplify resource_area uses in ClassPathDirEntry::open_stream

[David Holmes]

On Mon, 7 Nov 2022 17:17:35 GMT, Xin Liu <xliu at openjdk.org> wrote:

> Currently it is correct, but there are two places we can improve it.
> 
> 1. We should assert that current == Thread::current(). If it were not case, then we would intermix NEW_RESOURCE_ARRAY_IN_THREAD(current, char, path_len) and FREE_RESOURCE_ARRAY(char, path, path_len)!
> 
> 2. We don't need to call FREE_RESOURCE_ARRAY(char, path, path_len) for path in its first exit. It won't release anything because buffer stands in its way.
> 
> The resource area is like this. `Afree(path, path_len)` won't reclaim the space.
> 
> |path | buffer |...|
>                ^hwm

I'm okay with this PR if the new assert is dropped and you add a comment where the free was removed.

>  I wonder why allocation using 'current' but free using 'Thread::current()'

Good question! That is an asymmetry in the resourceArea API's. There should be a resource_free_bytes that takes the current thread as argument to save the Thread::current call.

> ClassPathDirEntry::open_stream is a public member function.

Sure but it still has very few callers and this is not a general purpose library class, so within this subsystem we trust our callers.

> Unfortunately, hotspot can't detect this error

I agree that is unfortunate. Returning a resource-allocated object is fragile.

> The reason I use NEW_RESOURCE_ARRAY_IN_THREAD(current, ... )  ...

Yes that was a good change. Seemed an oversight when it was already being used in the first allocation.

Thanks.

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

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