RFR(xs): 8151993: Remove inclusion of inline.hpp in log.hpp
Kim Barrett
kim.barrett at oracle.com
Thu Mar 17 22:33:49 UTC 2016
> On Mar 17, 2016, at 2:44 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
> On 03/16/2016 05:13 PM, Kim Barrett wrote:
>>> On Mar 16, 2016, at 8:33 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
>>>
>>> Hi, please review this small change.
>>>
>>> This also change allocation methods.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151993/
>>> Webrev: http://cr.openjdk.java.net/~rehn/8151993/webrev/
>>>
>>> Thanks!
>>>
>>> /Robbin
>>
>> ------------------------------------------------------------------------------
>> src/share/vm/logging/log.hpp
>> Changing this:
>> 138 char* newbuf = NEW_C_HEAP_ARRAY(char, newbuf_len, mtLogging);
>> to this:
>> 137 char* newbuf = (char*) os::malloc(sizeof(char) * newbuf_len, mtLogging);
>>
>> New code is missing out of memory handling that was present in the old
>> code. New code will just try to use newbuf, with bad results if the
>> allocation failed.
>
> Yes
>
>>
>> New code is missing ASSERT-conditionalized PrintMallocFree support
>> that was present in the old. I don't know how important this is,
>> given that we also have PrintMalloc. (And I have to wonder why we
>> have both PrintMalloc (develop) and PrintMallocFree (notproduct).)
>>
>> ------------------------------------------------------------------------------
>>
>
> I wonder if this is even is the right path, we have 25 hpp including allocation.inline.
> The other ones seem to do their allocation from their inline.hpp file.
>
> Making me believe that we should either ignore this or do log.inline.hpp.
>
> Either way this patch should be dropped, thoughts?
I don't think ignoring is the right long-term approach, though is
probably ok in the short term. In particular, I think adding
log.inline.hpp would touch a lot of files, which seems like a bad
thing to do just now, with largish changes coming soon that would need
to be merged. And I'm not convinced log.inline.hpp is the right long
term approach anyway.
Rather, I prefer Carsten's suggestion of an out of line slow-path
function in a .cpp. This approach is possible, even though we're
dealing with templates, by using type-erasure. The generic
(non-templated) slow-path function would need to receive a couple of
function pointers from the vwrite template, e.g. the prefix and puts
functions that are referenced by the present template code. The
function pointer types don't refer to template parameters, e.g. the
prefix function's type is
size_t (*prefix_fn)(char*, size_t)
and puts is
void (*puts_fn)(LogLevelType, const char*)
More information about the serviceability-dev
mailing list