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

Changes requested by dholmes (Reviewer).

src/hotspot/share/classfile/classLoader.cpp line 241:

> 239: 
> 240: ClassFileStream* ClassPathDirEntry::open_stream(JavaThread* current, const char* name) {
> 241:   assert(current == Thread::current(), "current is not equal to Thread::current()");

I understand the defensive programming philosophy, but the whole point of naming the parameter `current` is because it _is_ the current thread. We did a lot of cleanup of these JavaThread parameters (use of THREAD, CHECK etc) to make such things clear and to avoid the need to assert in every call that expects to be passed the current thread. If an API is called externally an assert may be warranted, but otherwise you would put such asserts in every single method that takes the current thread as a parameter and that would be ridiculously redundant. If a thread other than the current thread ever managed to sneak in here it would crash pretty quickly.

src/hotspot/share/classfile/classLoader.cpp line 263:

> 261:           ClassLoader::perf_sys_classfile_bytes_read()->inc(num_read);
> 262:         }
> 263:         FREE_RESOURCE_ARRAY(char, path, path_len);

I see your argument about this being basically a no-op, but we also lose the guard against access-after-free (ZapResourceArea). At a minimum I'd add a comment "// No need to free path here" else someone will think it is an oversight.

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

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