RFR: 8292989: Avoid dynamic memory in AsyncLogWriter [v4]
Xin Liu
xliu at openjdk.org
Mon Sep 12 14:16:28 UTC 2022
On Thu, 8 Sep 2022 11:15:21 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
>> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
>>
>> fix MSVC warning C4267
>>
>> MSVC reports logAsyncWriter.cpp(62): warning C4267: 'initializing': conversion from
>> 'size_t' to 'int', possible loss of data
>
> src/hotspot/share/logging/logAsyncWriter.hpp line 109:
>
>> 107: _capacity = value;
>> 108: return old;
>> 109: }
>
> Actually, I would prefer it if you did not shrink the buffer dynamically, since this is not a supported scenario and may cause unforeseen problems (besides making the code harder to argue about).
>
> You could just leave buffer size immutable after creation and instead add another test variation to your `AsyncLogGtest.java` where you start with a tiny tiny buffer size, and then run all your gtests with it. More test coverage, and we don't need the test coding here.
>
> (Side note, I see that in AsyncLogGtest.java you run all tests inside a single `@test` scope. If you split them up and give them speaking id's, these tests can run in parallel. For examples, see e.g. the NMTGtests. The duplication of the test section is annoying, but the parallelization of tests you get for free is cool. Also, you can start them individually if you want to reproduce a test error.)
if we want to test 'tiny-buffer' elegantly, we have to lift the constraint of `AsyncLogBufferSize`. I just feel that we shouldn't leave a door for testing so end-user can mess with it.
Code like this is a compromise. I think it's fine because Buffer isn't designed as a reusable component. We might be over-engineering if we try to tide up everything. What do you think?
-------------
PR: https://git.openjdk.org/jdk/pull/10092
More information about the hotspot-runtime-dev
mailing list