RFR(xs): 8151993: Remove inclusion of inline.hpp in log.hpp

[Robbin Ehn]

Hi Kim,

On 03/17/2016 11:33 PM, Kim Barrett wrote:
>> 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.

Ok,

>
> 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*)
>
>

I wanted to avoid introducing a lot of things not directly related to 
removing the include.

Thanks for your input!

/Robbin