[prev in list] [next in list] [prev in thread] [next in thread] 

List:       log4j-dev
Subject:    Re: [log4j-dev] RE: circular buffering (attached)
From:       Ken Arnold <arnold () moonhill ! org>
Date:       2002-08-08 17:37:51
[Download RAW message or body]

On Thursday, August 8, 2002, at 09:19  AM, Shapira, Yoav wrote:

> This topic should be moved to log4j dev probably.  Anyways, I looked
> over the code a tiny bit, a couple of things come to mind.

I take your point -- I have moved the discussion over from log4j-user

> - Use of Priority throughout: should be Level as this is future looking.

Oops, my bad.

> - Not sure I like the logic of "if a test is not specified, it is
> considered failed" in the Trigger.java class.  You could see a case
> where you wanted all levels to pass.  Maybe it would be better to set
> level at DEBUG initially, or require it be set.  Maybe only provide one
> constructor that requires both arguments (level and string to match)?

I think the language may be misleading.  I want to let you trigger on 
either level or string match.  Because it is an "or" test, I want to 
consider an unspecified level threshold level to cause the level test to 
fail.  If I considered it to have succeeded, an unspecified level would 
mean that every log event would be a trigger.  Given (levelMatch || 
stringMatch), an unspecified level match must fail.  And similarly for 
the string match.

> - The error message in BufferingAppender.checkClosed() could be nicer ;)

Hey, you abuse the system, it complains!-)

> - Why is getBufferSize() in BufferingAppender synchronized?

Because setBufferSize must be synchronized.  This is the way the Java 
memory model works, I'm afraid.  In principle if one thread doesn't 
synchronize it might live with an older value returned by getBufferSize 
forever.  (In practice few VMs have much of a thread-local cache and so 
it tends not to get stale, but I don't invent these things, I just work 
with 'em.-)

> - The Visitor pattern, in my experience, is great for designs where
> performance on a large scale is not required.  It's a great abstraction,
> and even works great in practice with small sets of objects.  However,
> I'm curious to see how it would perform in a system with, say, 5000
> classes (each with its own logger) and frequent visitations.  Have you
> done any performance testing on this scheme?

I have viewed the buffer flushing operation as relatively rare -- when 
an error occurs, flush the buffers.  It is *not* meant to be something 
happening frequently or there is no benefit to having anything buffered 
because most messages will be sent through anyway.  As such, the 
performance penalty is probably acceptable.  (The only way I know of to 
make this faster is to change the tree to contain references to 
children, not just parents, and the overhead of maintaining the two-way 
linkage seems to outweigh the benefit.  Maybe I'm missing something.)

> - In LoggerNavigator, at least the version I have, vistAllAppenders
> should probably be visitAllAppenders ;)

My bad again.

> - I prefer while(true) to for(; ;), but that may be a personal
> preference.  Anything to make the code easier to read though.

Well, I've been using "for (;;)" since before most of you were dry in 
your diapers.  Which proves nothing, of course, except that I'm getting 
to be cranky old guy.  Anyway, "while (true)" always seemed to have this 
odd implication that "true" might become "false" someday, but "for (;;)" 
says "I'm not testing for anything."  But if there is a coding style on 
this for log4j, don't let me stand in the way.

> - The |= is cool.  ;)  Haven't seen that one in a while.

See previous reference to "cranky old guy".-)

> I'd say my biggest concern is the performance right now.  There are a
> bunch of synchronized() methods, and the Visitor pattern, to consider.
> I'd like to see JUnit tests provided for this code, as well as
> performance comparisons (logging with, without a BufferingAppender on a
> JVM with N loggers, X logging calls / per second, increase N and X,
> etc).

On synchronization, I live in a heavily multithreaded world so I tend to 
be paranoid in this area.  Calls to get/setBufferSize are probably too 
rare to matter, as are the calls that manipulate the appender list.  I 
presume that the only method called enough to matter in performance is 
append, but if it isn't synchronized and logging comes through multiple 
threads, messages will otherwise be lost, so it seems necessary.  Or am 
I missing some other mechanism/custom?

> Regardless of whether this makes the codebase now, eventually, or never,
> thank you Ken for taking the time and contributing! ;)

Happy to.  Thanks for looking at it.

		Ken Arnold


--
To unsubscribe, e-mail:   <mailto:log4j-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:log4j-dev-help@jakarta.apache.org>

[prev in list] [next in list] [prev in thread] [next in thread]