possible race condition in JsonFormatter?

[quarkiverse]

possible race condition in JsonFormatter?

[SlyngDK]

@dfa1 Thx for creating the issue, I understand the problem you are pointing at.

I see these different solutions for the problem, with different performance impact.

• synchronized like the class the JsonFormatter extends StructuredFormatter
• Pool of writers, result in some other locking when requesting at writer not in use.
• Use new writer for each format, results in more object creation(Including auto expand of buffer) and GC.

I don't have any performance numbers of the different solutions, but I think synchronized is the best compromise.

@dfa1 If you have the time you could create a micro benchmark, to check the numbers, against current library.

[SlyngDK]

I have done a quick benchmark with 1 and 2 thread, and creating a new StringBuildWriter is the best compromise.

I have released 1.0.1.

Benchmark                        Mode  Cnt        Score   Error  Units
BenchmarkNewStringWriter.init1  thrpt    2   598932,618          ops/s
BenchmarkNewStringWriter.init2  thrpt    2  1087659,986          ops/s
BenchmarkObjectPool.init1       thrpt    2   554206,536          ops/s
BenchmarkObjectPool.init2       thrpt    2   837947,587          ops/s
BenchmarkOld.init1              thrpt    2   670263,357          ops/s
BenchmarkOld.init2              thrpt    2  1206957,338          ops/s
BenchmarkSynchronized.init1     thrpt    2   663996,158          ops/s
BenchmarkSynchronized.init2     thrpt    2   577327,380          ops/s

[gsmet]

You can play for sure but my advice is to keep it simple. StringBuilder is not an expensive object to create and it has been designed for exactly this usage.

[dfa1]

@gsmet please have a look here. A bit of context: I just migrated to this library for our JSON logging needs, as per quarkusio/quarkus#15193 (comment)

Often in Splunk we have corrupted logs starting with t{, instead of {

[dfa1]

/cc @SlyngDK maybe you can help here?

[dfa1]

@SlyngDK thank you for the answer :) I would like to have a release as soon as possible, so maybe the synchronized is the best option right now (better than my proposed PR).

@SlyngDK later I could try to write a benchmark, also to evaluate the option of pool of writers (e.g. I would like to try with this little object pool library requiring only JDK classes, https://github.com/chrisvest/stormpot)

[gsmet]

@SlyngDK I would just create a new one each time. It's a lightweight object and that's how you use StringBuilder usually.

[dfa1]

@SlyngDK would it be possible to merge my pull request as is, release it and then iterate on a better fix? @gsmet has a valid argument about StringBuilder)

[dfa1]

thank you @SlyngDK!

just one question: which object pooling library you used in the benchmark?

[SlyngDK]

@dfa1 I used Apache Commons Pool.

[dfa1]

@SlyngDK thank you! Can you please push the benchmark code somewhere? I would like to play a bit with https://github.com/chrisvest/stormpot

[dfa1]

@gsmet yes, it is just out of curiosity :) By the way the current fix shipped with 1.0.1 is obviously working fine for my use case (few thousands log events per hour)