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

[Xin Liu]

On Tue, 8 Nov 2022 05:49:11 GMT, David Holmes <dholmes 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).

hi, @dholmes-ora 

Thank you for reviewing this. I am okay to withdraw this PR. 

I just feel confused in this method when I hunt an arena/chunk bug. Maybe we can improve it using comment? 

1. It appears a mismatch. I wonder why allocation using 'current' but free using 'Thread::current()'. An assertion here serves as a cognitive precondition. If hotspot runtime asserts this in earlier places, I think a comment is fine. `ClassPathDirEntry::open_stream` is a public member function. We shouldn't assume future readers/users of it equip with the same level knowledge like you :)

2. ResourceMark isn't allowed in this method because `buffer=NEW_RESOURCE_ARRAY_IN_THREAD(...)` is part of return value! If I placed ResourceMark, it would reclaim buffer before this function returns. Unfortunately, hotspot can't detect this error. It just mysteriously fails to load classes from zip files. A lot of guesswork required to figure out why. 

3. The reason I use NEW_RESOURCE_ARRAY_IN_THREAD(current, ... ) and delete FREE_RESOURCE_ARRAY because I think we can make it **a little bit** faster in runtime.  eg. using `current` directly is supposed to faster than Thread::current(), but I admit that we shouldn't trade it with readability. It's not worth it. 

For above concerns, should I replace code change with comments or just drop this PR? 

thanks,
--lx

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

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