[jira] [Comment Edited] (LOG4J2-585) Markers not as powerful as slf4j

["Bruce Brouwer (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/LOG4J2-585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13958801#comment-13958801 ] 

Bruce Brouwer edited comment on LOG4J2-585 at 4/3/14 1:24 PM:
--------------------------------------------------------------

Here is a log4j2-585-concept.patch that outlines my ideas for the Markers. It is merely a concept and not intended to be committed yet. I'm sure I've probably gone too far in some areas, but hear me out first. 

For performance, I checked with JMH. So long as the marker hierarchy is no more than 5 deep, the current Log4j code is faster, but after 5 deep, my concept is faster than the current Log4j code. For the normal case of only one parent, the current Log4j code can perform about 600,000 ops/ms, while my concept performs about 200,000 ops/ms. A simpler version of my concept that did not include the allParentNames variable was much slower, clocking in at only about 10,000 ops/ms. 

The big driver behind my concept was to make Log4j Markers just as powerful as slf4j. This means primarily two things: mutability and multiple parents. 

First, I got rid of MarkerManager and turned Marker into a concrete, final class. By making Marker an interface, clients might be inclined to implement the interface themselves and cause issues because the parent hierarchy of their markers would likely not match the parent hierarchy of Log4j's markers. It is because of this that I don't like the idea of pluggable Marker factories. 

Next, my ReadWriteLock is static. I did this to avoid creating a ton of lock objects for each Marker. The majority of time, marker hierarchies are not changing, so a lock that blocks most marker functions during an update is likely not a concern. Furthermore, the most prevalent cases, such as calling isInstanceOf, are not even guarded by a lock and allow ConcurrentHashMap to provide all the performance possible.

getParents() is now a Set<Marker> instead of a simple Marker. This allows multiple parents. I went with a custom Set implementation to be able to react to changes to the parents set, which is used primarily to keep allParentNames up to date. 

The static methods that used to be on MarkerManager are now on Marker. 

Marker.get works exactly like MarkerManager.getMarker. An overloaded version allows you to indicate if you want a marker created if it does not exist. This was necessary for use in the Slf4j-impl, but I could see it being valuable elsewhere, too.

Marker.define works a little bit like MarkerManager.getMarker(name, parent), but because Markers are now mutable, it ensures that the parents become the list provided here. I like the name define better because it is clear to me that this will make the marker look the way I just described. I did not like MarkerManager.getMarker(name, parent) because it does not guarantee me that the returned marker even has the specified parent. If the marker was created earlier to this call, it won't have my specified parent. 

Marker.undefine allows markers to be removed, which is a feature of slf4j and it was fairly easy to support here. 

So, is it worth a bit of a performance penalty to get all these features in Markers? 


was (Author: bruce.brouwer):
Here is a log4j2-585-concept.patch that outlines my ideas for the Markers. It is merely a concept and not intended to be committed yet. I'm sure I've probably gone too far in some areas, but hear me out first. 

For performance, I checked with JMH. So long as the marker hierarchy is no more than 5 deep, the current Log4j code is faster, but after 5 deep, my concept is faster than the current Log4j code. For the normal case of only one parent, the current Log4j code can perform about 600,000 ops/ms, while my concept performs about 200,000 ops/ms. A simpler version of my concept that did not include the allParentNames variable was much slower, clocking in at only about 10,000 ops/ms. 

The big driver behind my concept was to make Log4j Markers just as powerful as slf4j. This means primarily two things: mutability and multiple parents. 

First, I got rid of MarkerManager and turned Marker into a concrete, final class. By making Marker an interface, clients might be inclined to implement the interface themselves and cause issues because the parent hierarchy of their markers would likely not match the parent hierarchy of Log4j's markers. It is because of this that I don't like the idea of pluggable Marker factories. 

Next, my ReadWriteLock is static. I did this to avoid creating a ton of lock objects for each Marker. The majority of time, marker hierarchies are not changing, so a lock that blocks most marker functions during an update is likely not a concern. Furthermore, the most prevalent cases, such as calling isInstanceOf, are not even guarded by a lock and allow ConcurrentHashMap to provide all the performance possible.

getParents() is not a Set<Marker> instead of a simple Marker. This allows multiple parents. I went with a custom Set implementation to be able to react to changes to the parents set, which is used primarily to keep allParentNames up to date. 

The static methods that used to be on MarkerManager are not on Marker. 

Marker.get works exactly like MarkerManager.getMarker. An overloaded version allows you to indicate if you want a marker created if it does not exist. This was necessary for use in the Slf4j-impl, but I could see it being valuable elsewhere, too.

Marker.define works a little bit like MarkerManager.getMarker(name, parent), but because Markers are not mutable, it ensures that the parents become the set provided here. I like the name define better because it is clear to me that this will make the marker look the way I just described. I did not like MarkerManager.getMarker(name, parent) because it does not guarantee me that the returned marker even has the specified parent. If the marker was created earlier to this call, it won't have my specified parent. 

Marker.undefine allows markers to be removed, which is a feature of slf4j and it was fairly easy to support here. 

So, is it worth a bit of a performance penalty to get all these features in Markers? 

> Markers not as powerful as slf4j
> --------------------------------
>
>                 Key: LOG4J2-585
>                 URL: https://issues.apache.org/jira/browse/LOG4J2-585
>             Project: Log4j 2
>          Issue Type: Improvement
>          Components: API
>    Affects Versions: 2.0-rc1
>            Reporter: Bruce Brouwer
>         Attachments: log4j2-585-concept.patch
>
>
> Log4J's markers are not as flexible as markers in SLF4J. 
> First, SLF4J's markers are mutable. By allowing markers to be mutable, I can change the relationship of markers to each other based upon runtime or business conditions. 
> Second, and more importantly I think, is that essentially SLF4J markers have this parent/child relationship, much like Log4J, except that in SLF4J, I can essentially have a marker with multiple parents. For example, I might want this structure:
> * Animal
> ** Bird
> *** Duck
> ** Mammal
> *** Bear
> *** Dolphin
> * Travels by
> ** Water
> *** Duck
> *** Dolphin
> ** Land
> *** Duck
> *** Bear
> ** Air
> *** Duck
> Of course, this is a contrived example, but I wanted to describe the relationships. Now, if I wanted to filter based on markers that travel by Water for some appenders, and another appender wants to filter by Mammals, I can't simply use the single marker of Dolphin. 
> Either we need to reverse the marker relationship so that it contains its children, much like SLF4J, or we allow markers to have multiple parents, which I prefer because it could make it more succinct to define:
> {code}
> private static final Marker BY_LAND = MarkerManager.getMarker("BY_LAND");
> private static final Marker BY_WATER = MarkerManager.getMarker("BY_WATER");
> private static final Marker DUCK = MarkerManager.getMarker("DUCK", BY_LAND, BY_WATER);
> {code}
> As for the Marker API, we would either need to change getParent to getParents, or get rid of the getParent method from the API and just rely on the isInstanceOf method to handle checking multiple parents by looking at private member variables (my preference)



--
This message was sent by Atlassian JIRA
(v6.2#6252)

---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org