You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Volkan Yazıcı <vo...@gmail.com> on 2020/03/15 10:37:23 UTC
LockingStringBuilderEncoder ByteBuffer
Hello,
I want to take advantage of StringBuilder plumbing in the core module.
There are two Encoder<StringBuilder> implementations:
StringBuilderEncoder and LockingStringBuilderEncoder. Given
JsonTemplateLayout has its own customizable recycling strategy, I
can't use StringBuilderEncoder due to its TLAs.
LockingStringBuilderEncoder, even though I cannot find any usage of
this class, contains the following interesting bit:
// This synchronized is needed to be able to call destination.getByteBuffer()
synchronized (destination) {
TextEncoderHelper.encodeText(charsetEncoder, cachedCharBuffer,
destination.getByteBuffer(), source,
destination);
}
Why doesn't there exist a ByteBuffer class field but rather it relies
on destination.getByteBuffer()? This would have eliminated the need
for the synchronized block. For instance, StringBuilderEncoder has its
own thread-local ByteBuffer and avoids the synchronized.
Kinds regards.
Re: LockingStringBuilderEncoder ByteBuffer
Posted by Ralph Goers <ra...@dslextreme.com>.
> On Mar 15, 2020, at 2:10 PM, Volkan Yazıcı <vo...@gmail.com> wrote:
>
> On Sun, Mar 15, 2020 at 6:03 PM Ralph Goers <ra...@dslextreme.com> wrote:
>> After looking at the code I am sure there is something
>> wrong with the logic. While you can set property
>> log4j2.enable.direct.encoders to true or false it
>> uses the same Encoder in either case.
>
> I am a little bit puzzled here. Which particular "logic" are you
> referring to exactly? The following in AbstractStringLayout?
AbstractLayout has
public void encode(final LogEvent event, final ByteBufferDestination destination) {
final byte[] data = toByteArray(event);
destination.writeBytes(data, 0, data.length);
}
PaternLayout implements this as
@Override
public void encode(final LogEvent event, final ByteBufferDestination destination) {
if (!(eventSerializer instanceof Serializer2)) {
super.encode(event, destination);
return;
}
final StringBuilder text = toText((Serializer2) eventSerializer, event, getStringBuilder());
final Encoder<StringBuilder> encoder = getStringBuilderEncoder();
encoder.encode(text, destination);
trimToMaxSize(text);
}
>> As for why it is not a class member variable, that would
>> be because it wouldn’t be thread-safe if it was. The encoder
>> is a member of PatternLayout and multiple threads can be
>> using the same PatternLayout at the same time. That said,
>> even then I’m not sure why it has to be synchronized. Will
>> the ByteBuffer be manipulated from other threads while the
>> Layout is being rendered?
>
> That was exactly the question in my mind. I had the impression that
> TextEncoderHelper.encodeText() is supposed to take the necessary
> synchronization measures.
>
> BTW, LockingStringBuilderEncoder is not used anywhere, right? Am I
> missing something?
I also don’t see it used anywhere.
>
> Speaking of ENABLE_DIRECT_ENCODERS flag, what does it exactly mean
> when it's set to false? That is, if my layout can perfectly drain its
> output into a ByteBufferDestination without any TLAs or such, what is
> the point of disabling it because user has just requested that?
I would have assumed it would have allowed for different implementations. But using true/false isn’t the way to do that. I didn’t implement this so I am not sure why it exists as it does.
Ralph
Re: LockingStringBuilderEncoder ByteBuffer
Posted by Volkan Yazıcı <vo...@gmail.com>.
On Sun, Mar 15, 2020 at 6:03 PM Ralph Goers <ra...@dslextreme.com> wrote:
> After looking at the code I am sure there is something
> wrong with the logic. While you can set property
> log4j2.enable.direct.encoders to true or false it
> uses the same Encoder in either case.
I am a little bit puzzled here. Which particular "logic" are you
referring to exactly? The following in AbstractStringLayout?
protected AbstractStringLayout(
final Charset aCharset,
final byte[] header,
final byte[] footer) {
// ...
textEncoder = Constants.ENABLE_DIRECT_ENCODERS
? new StringBuilderEncoder(charset)
: null;
}
protected Encoder<StringBuilder> getStringBuilderEncoder() {
if (textEncoder == null) {
textEncoder = new StringBuilderEncoder(getCharset());
}
return textEncoder;
}
If so, what should have been the correct logic?
> As for why it is not a class member variable, that would
> be because it wouldn’t be thread-safe if it was. The encoder
> is a member of PatternLayout and multiple threads can be
> using the same PatternLayout at the same time. That said,
> even then I’m not sure why it has to be synchronized. Will
> the ByteBuffer be manipulated from other threads while the
> Layout is being rendered?
That was exactly the question in my mind. I had the impression that
TextEncoderHelper.encodeText() is supposed to take the necessary
synchronization measures.
BTW, LockingStringBuilderEncoder is not used anywhere, right? Am I
missing something?
Speaking of ENABLE_DIRECT_ENCODERS flag, what does it exactly mean
when it's set to false? That is, if my layout can perfectly drain its
output into a ByteBufferDestination without any TLAs or such, what is
the point of disabling it because user has just requested that?
Re: LockingStringBuilderEncoder ByteBuffer
Posted by Ralph Goers <ra...@dslextreme.com>.
After looking at the code I am sure there is something wrong with the logic. While you can set property log4j2.enable.direct.encoders to true or false it uses the same Encoder in either case.
As for why it is not a class member variable, that would be because it wouldn’t be thread-safe if it was. The encoder is a member of PatternLayout and multiple threads can be using the same PatternLayout at the same time. That said, even then I’m not sure why it has to be synchronized. Will the ByteBuffer be manipulated from other threads while the Layout is being rendered? That would require asynchronous pattern converters.
Ralph
> On Mar 15, 2020, at 3:37 AM, Volkan Yazıcı <vo...@gmail.com> wrote:
>
> Hello,
>
> I want to take advantage of StringBuilder plumbing in the core module.
> There are two Encoder<StringBuilder> implementations:
> StringBuilderEncoder and LockingStringBuilderEncoder. Given
> JsonTemplateLayout has its own customizable recycling strategy, I
> can't use StringBuilderEncoder due to its TLAs.
> LockingStringBuilderEncoder, even though I cannot find any usage of
> this class, contains the following interesting bit:
>
> // This synchronized is needed to be able to call destination.getByteBuffer()
> synchronized (destination) {
> TextEncoderHelper.encodeText(charsetEncoder, cachedCharBuffer,
> destination.getByteBuffer(), source,
> destination);
> }
>
> Why doesn't there exist a ByteBuffer class field but rather it relies
> on destination.getByteBuffer()? This would have eliminated the need
> for the synchronized block. For instance, StringBuilderEncoder has its
> own thread-local ByteBuffer and avoids the synchronized.
>
> Kinds regards.
>