stringStream in UL and nested ResourceMarks

[Marcus Larsson]

On 2017-06-12 17:47, Thomas Stüfe wrote:
>
>
> Okay, I put a bit of work into it (stack-only LogStream) and 
> encountered nothing difficult, just a lot of boilerplate work.
>
> This is how I am changing the callsites:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8181917-UL-should-not-use-resource-memory-for-LogStream/current-work/webrev/ 
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-UL-should-not-use-resource-memory-for-LogStream/current-work/webrev/>
>
> Please tell me if you had something different in mind; I want to make 
> sure we have the same idea before changing such a lot of code.

Nice work. This is what I expected to see.

Just a small suggestion I thought of when looking at it:

Instead of

769 Log(itables) logi;
770 LogStream lsi(logi.trace());

you could do

769 LogStream lsi(Log(itables)::trace());

to save a line.
Of course this only works if the log instance isn't used for anything 
else than initializing the LogStream.

Touching on this is a suggestion that was brought up here when 
discussing this locally, which is to add is_enabled() functions to the 
LogStream class.
By doing that we wouldn't need the log instance at all in places where 
only the stream API is used, which is good.
If we're going for that, it would be great to do it before this change 
so we could fix up the call sites to use that as well.
Let me know what you think.

>
> Notes:
> - I only remove ResourceMark objects where I can be sure that they are 
> intended to scope the LogStream itself. If I cannot be sure, I leave 
> them in for now.
> - using LogStream as a stack object means we need to include 
> logStream.hpp (or decide to move LogStream to log.hpp).

I don't think it's a big deal to include logStream.hpp as well, but we 
can consider moving it into log.hpp if people prefer that. I don't 
really have a strong opinion on this.

> - If there is only one debug level involved (true in a lot of places), 
> I prefer LogTarget to Log, because it minimizes error possibilities
> - In some places the coding gets more complicated, usually if the code 
> wants to decide the log level dynamically, but the log level is a 
> template parameter to LogTarget. See e.g. linkresolver.cpp.
> - I see the macro "log_develop_is_enabled" and wonder why it exists 
> and why the code is not just surrounded by #ifndef PRODUCT?

That's only for convenience. It could as well have been ifndef:ed.

Thanks,
Marcus