You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by David Blevins <db...@tomitribe.com> on 2022/04/21 01:04:39 UTC
Showing a chunk of json in exceptions
Hey All,
With inspiration from Romain, I went ahead and wrote a Json serializer that can create short Json snippets that we can safely include in error messages. I.e. it is optimized to only write a specific number of bytes and then stop. The recursion is tightly controlled to stop early as well. You can see that code in this PR if you're curious:
- https://github.com/apache/johnzon/pull/84
A few thoughts on where we could go from here:
- We can probably work this into more exception handling that currently doesn't print Json
- We do have some existing exception handling calling toString on JsonValue instances we should update. For example:
https://github.com/apache/johnzon/blob/v1.2.16/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/MappingParserImpl.java#L627
- The current default number of characters in a snippet is 50. We could potentially have a system property to allow users to get more.
Thoughts?
-David
Re: Showing a chunk of json in exceptions
Posted by Jean-Louis Monteiro <jl...@tomitribe.com>.
I agree errors aren't always very useful for end users. So anything in that
area is a good addition and improvement.
Thanks David
--
Jean-Louis Monteiro
http://twitter.com/jlouismonteiro
http://www.tomitribe.com
On Thu, Apr 21, 2022 at 8:13 AM Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Le jeu. 21 avr. 2022 à 03:04, David Blevins <db...@tomitribe.com> a
> écrit :
>
> > Hey All,
> >
> > With inspiration from Romain, I went ahead and wrote a Json serializer
> > that can create short Json snippets that we can safely include in error
> > messages. I.e. it is optimized to only write a specific number of bytes
> > and then stop. The recursion is tightly controlled to stop early as
> well.
> > You can see that code in this PR if you're curious:
> >
> > - https://github.com/apache/johnzon/pull/84
>
>
> Commented a few details but it is overall *the* solution! Thanks David!
>
>
> >
> > A few thoughts on where we could go from here:
> >
> > - We can probably work this into more exception handling that currently
> > doesn't print Json
> >
>
>
> +1
>
>
> > - We do have some existing exception handling calling toString on
> > JsonValue instances we should update. For example:
> >
> >
> https://github.com/apache/johnzon/blob/v1.2.16/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/MappingParserImpl.java#L627
>
>
> Outch, assumption was that it was a primitive but you are right it is wrong
>
>
> >
> > - The current default number of characters in a snippet is 50. We could
> > potentially have a system property to allow users to get more.
> >
>
> Nop, a JsonXxFactory property as all toggles ;)
> +1 for it
>
>
>
> >
> > Thoughts?
> >
> >
> > -David
> >
> >
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
> On Apr 25, 2022, at 1:27 PM, David Blevins <da...@gmail.com> wrote:
>
>> On Apr 25, 2022, at 12:23 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>>
>> Oh, got it, thanks for re-explaining.
>>
>> Did you try a simple heuristic on visiting to estimate the size (3 chars
>> for a number, no escaping for string, just length etc...), should enable to
>> cut fast enough the visiting without hacking lower level any buffer
>> strategy or buffer nor calling flush and moving the array data too often?
>> Otherwise we can do a custom generator factory from the provider in mapper
>> propagting properties but Im less a fan of that option cause it complexify
>> the user customization of the generator (today we can output other stuff
>> than json through that way).
>
> The most optimized solution I can image is setting the buffer size of JsonGeneratorImpl to the snippet size, then adding an option to JsonGeneratorImpl so that it can be told to call flush() on the Writer when it reaches its buffer limit.
If we're being honest with ourselves here as well, copying the config map and updating it is likely to itself add more array copies, object reference updates, iterating over bytes in strings used as keys during map inserts, etc.
-David
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
> On Apr 25, 2022, at 12:23 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Oh, got it, thanks for re-explaining.
>
> Did you try a simple heuristic on visiting to estimate the size (3 chars
> for a number, no escaping for string, just length etc...), should enable to
> cut fast enough the visiting without hacking lower level any buffer
> strategy or buffer nor calling flush and moving the array data too often?
> Otherwise we can do a custom generator factory from the provider in mapper
> propagting properties but Im less a fan of that option cause it complexify
> the user customization of the generator (today we can output other stuff
> than json through that way).
The most optimized solution I can image is setting the buffer size of JsonGeneratorImpl to the snippet size, then adding an option to JsonGeneratorImpl so that it can be told to call flush() on the Writer when it reaches its buffer limit.
That said, I think we should shift focus to getting this used by as many exceptions as we can and updating the documentation then come back to it.
With the default snippet size of 50 no real harm can be done by the flushes.
My $0.02 at least.
-David
>
> Le lun. 25 avr. 2022 à 19:47, David Blevins <da...@gmail.com> a
> écrit :
>
>>> On Apr 24, 2022, at 11:09 PM, Romain Manni-Bucau <rm...@gmail.com>
>> wrote:
>>>
>>> Le dim. 24 avr. 2022 à 22:30, David Blevins <da...@gmail.com> a
>>> écrit :
>>>
>>>> All,
>>>>
>>>> I added more tests and found that most the optimizations were not
>>>> happening due to buffering.
>>>>
>>>> Essentially there are two buffers between Snippet.Buffer and
>>>> Snippet.SnippetOutputStream. The SnippetOutputStream had the
>>>> responsibility to tell the code up the stack when we've reached the max
>>>> snippet length. Since all the bytes were buffered, it would see nothing
>>>> until the very end and we'd end up serializing the full json text
>> anyway.
>>>>
>>>> One is the 64k buffer in JsonGeneratorImpl and the other is an 8k buffer
>>>> in the JVM implementation code of OutputStreamWriter. Since the
>>>> OutputStreamWriter buffer is hardcoded, we can't solve this by adjusting
>>>> buffer sizes and have no choice but to aggressively call flush() to
>> ensure
>>>> SnippetOutputStream has the bytes and can do its job.
>>>>
>>>
>>> Not sure I get that since if you close in a finally block the generator,
>> it
>>> will flush the actual output and all will be good.
>>> But can be to call tostring to early rather than a buffering issue
>>
>> It'd difficult to explain, but I'll do my best and thanks for the patience
>> if my attempt is poor.
>>
>> The code is designed with the assumption that as json is serialized there
>> will be write calls made on SnippetOutputStream, which then counts the
>> bytes and can eventually tell Snippet.Buffer to stop making more json via
>> the 'snippet.terminate()' calls. In practice this doesn't happen.
>>
>> In practice what does happen is the entire json document, up to a limit of
>> 64k, will be created before any calls reach SnippetOutputStream. This is
>> because JsonGeneratorImpl is holding a buffer (64k by default) and does not
>> call any writes on the Writer instance it's holding until that buffer has
>> filled or close is called.
>>
>> My first instinct was to reduce that 64k buffer to the snippet max length
>> and solve this problem that way. The trick with that is there is yet
>> another buffer being held internally by ObjectOutputStream and it recreates
>> the issue. That buffer is hardcoded to be 8k. So even if we adjust the
>> JsonGeneratorImpl buffer size, in practice what happens is the entire json
>> document, up to a limit of 8k, will be created before any calls reach
>> SnippetOutputStream.
>>
>> Certainly 8k is better than 64k which is better than potentially 1GB of
>> json, but I wanted to try and get close to the spirit of what we were both
>> after originally which is that we avoid serializing a lot of json only to
>> throw most of it away and show just a chunk of it.
>>
>> The only way to do that is the flush() calls. That's the only way to
>> ensure SnippetOutputStream is getting the json data as we serialize it.
>>
>> Hope some of this helps.
>>
>>
>> -David
>>
>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Oh, got it, thanks for re-explaining.
Did you try a simple heuristic on visiting to estimate the size (3 chars
for a number, no escaping for string, just length etc...), should enable to
cut fast enough the visiting without hacking lower level any buffer
strategy or buffer nor calling flush and moving the array data too often?
Otherwise we can do a custom generator factory from the provider in mapper
propagting properties but Im less a fan of that option cause it complexify
the user customization of the generator (today we can output other stuff
than json through that way).
Le lun. 25 avr. 2022 à 19:47, David Blevins <da...@gmail.com> a
écrit :
> > On Apr 24, 2022, at 11:09 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Le dim. 24 avr. 2022 à 22:30, David Blevins <da...@gmail.com> a
> > écrit :
> >
> >> All,
> >>
> >> I added more tests and found that most the optimizations were not
> >> happening due to buffering.
> >>
> >> Essentially there are two buffers between Snippet.Buffer and
> >> Snippet.SnippetOutputStream. The SnippetOutputStream had the
> >> responsibility to tell the code up the stack when we've reached the max
> >> snippet length. Since all the bytes were buffered, it would see nothing
> >> until the very end and we'd end up serializing the full json text
> anyway.
> >>
> >> One is the 64k buffer in JsonGeneratorImpl and the other is an 8k buffer
> >> in the JVM implementation code of OutputStreamWriter. Since the
> >> OutputStreamWriter buffer is hardcoded, we can't solve this by adjusting
> >> buffer sizes and have no choice but to aggressively call flush() to
> ensure
> >> SnippetOutputStream has the bytes and can do its job.
> >>
> >
> > Not sure I get that since if you close in a finally block the generator,
> it
> > will flush the actual output and all will be good.
> > But can be to call tostring to early rather than a buffering issue
>
> It'd difficult to explain, but I'll do my best and thanks for the patience
> if my attempt is poor.
>
> The code is designed with the assumption that as json is serialized there
> will be write calls made on SnippetOutputStream, which then counts the
> bytes and can eventually tell Snippet.Buffer to stop making more json via
> the 'snippet.terminate()' calls. In practice this doesn't happen.
>
> In practice what does happen is the entire json document, up to a limit of
> 64k, will be created before any calls reach SnippetOutputStream. This is
> because JsonGeneratorImpl is holding a buffer (64k by default) and does not
> call any writes on the Writer instance it's holding until that buffer has
> filled or close is called.
>
> My first instinct was to reduce that 64k buffer to the snippet max length
> and solve this problem that way. The trick with that is there is yet
> another buffer being held internally by ObjectOutputStream and it recreates
> the issue. That buffer is hardcoded to be 8k. So even if we adjust the
> JsonGeneratorImpl buffer size, in practice what happens is the entire json
> document, up to a limit of 8k, will be created before any calls reach
> SnippetOutputStream.
>
> Certainly 8k is better than 64k which is better than potentially 1GB of
> json, but I wanted to try and get close to the spirit of what we were both
> after originally which is that we avoid serializing a lot of json only to
> throw most of it away and show just a chunk of it.
>
> The only way to do that is the flush() calls. That's the only way to
> ensure SnippetOutputStream is getting the json data as we serialize it.
>
> Hope some of this helps.
>
>
> -David
>
>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Sounds good to me
Le ven. 29 avr. 2022 à 11:35, Mark Struberg <st...@yahoo.de.invalid> a
écrit :
> Means we are good to go and ship another release?
>
> Would love to do that and then update TomEE and Meecrowave with it (tests
> do look good in both).
> Wdyt?
> Would start the release train in 2h, love getting it out before the
> weekend.
>
> LieGrue,
> strub
>
> > Am 28.04.2022 um 08:27 schrieb David Blevins <da...@gmail.com>:
> >
> >> On Apr 26, 2022, at 10:55 AM, David Blevins <da...@gmail.com>
> wrote:
> >>
> >> I'd need to check on the character encoding issue you mention. In my
> mind the original code and current code is trying to create a string of max
> snippet length. If it doesn't do that, it's a bug.
> >
> > So I dug into this and it looks like counting bytes is very flawed and
> counting chars is as perfect as it gets in java.
> >
> > It looks like even with UTF-8 you can have a single character be
> anywhere from 1 to 4 bytes. The character `ñ` is string length of 1 but a
> byte length of 2. If you grabbed the first 3 bytes of "mañana" you'd get
> "ma�..."
> >
> > If you create a UTF-8 string from a four-byte UTF-8 character you get of
> course 4 bytes in the OutputStream, but you also get a string instance that
> claims to be of length 2 not 1. If you call substring(0,1) on that you get
> an unprintable result.
> >
> > So we fixed a bug in the switch from OutputStream to Writer. Any issues
> there are with counting chars passed to the Writer and shared by
> java.lang.String so users should not be surprised if they see a funny
> character at the end of the snippet sometimes.
> >
> >
> > -David
> >
> >
> >
>
>
Re: Showing a chunk of json in exceptions
Posted by Mark Struberg <st...@yahoo.de.INVALID>.
Means we are good to go and ship another release?
Would love to do that and then update TomEE and Meecrowave with it (tests do look good in both).
Wdyt?
Would start the release train in 2h, love getting it out before the weekend.
LieGrue,
strub
> Am 28.04.2022 um 08:27 schrieb David Blevins <da...@gmail.com>:
>
>> On Apr 26, 2022, at 10:55 AM, David Blevins <da...@gmail.com> wrote:
>>
>> I'd need to check on the character encoding issue you mention. In my mind the original code and current code is trying to create a string of max snippet length. If it doesn't do that, it's a bug.
>
> So I dug into this and it looks like counting bytes is very flawed and counting chars is as perfect as it gets in java.
>
> It looks like even with UTF-8 you can have a single character be anywhere from 1 to 4 bytes. The character `ñ` is string length of 1 but a byte length of 2. If you grabbed the first 3 bytes of "mañana" you'd get "ma�..."
>
> If you create a UTF-8 string from a four-byte UTF-8 character you get of course 4 bytes in the OutputStream, but you also get a string instance that claims to be of length 2 not 1. If you call substring(0,1) on that you get an unprintable result.
>
> So we fixed a bug in the switch from OutputStream to Writer. Any issues there are with counting chars passed to the Writer and shared by java.lang.String so users should not be surprised if they see a funny character at the end of the snippet sometimes.
>
>
> -David
>
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
> On Apr 26, 2022, at 10:55 AM, David Blevins <da...@gmail.com> wrote:
>
> I'd need to check on the character encoding issue you mention. In my mind the original code and current code is trying to create a string of max snippet length. If it doesn't do that, it's a bug.
So I dug into this and it looks like counting bytes is very flawed and counting chars is as perfect as it gets in java.
It looks like even with UTF-8 you can have a single character be anywhere from 1 to 4 bytes. The character `ñ` is string length of 1 but a byte length of 2. If you grabbed the first 3 bytes of "mañana" you'd get "ma�..."
If you create a UTF-8 string from a four-byte UTF-8 character you get of course 4 bytes in the OutputStream, but you also get a string instance that claims to be of length 2 not 1. If you call substring(0,1) on that you get an unprintable result.
So we fixed a bug in the switch from OutputStream to Writer. Any issues there are with counting chars passed to the Writer and shared by java.lang.String so users should not be surprised if they see a funny character at the end of the snippet sometimes.
-David
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Fixed https://issues.apache.org/jira/browse/JOHNZON-368 and added
https://issues.apache.org/jira/browse/JOHNZON-369
Tink last one can enable you to solve your issue
(check org.apache.johnzon.core.JsonGeneratorFactoryImplTest#boundedOutputStream
for the idea except in your case you already control the generator buffer
size so you just need to use the right Writer when johnzon is available -
fallback on a default impl when not there - if not clear: you can use
`SnippetWriter extends BoundedOutputStreamWriter implements Buffered` in a
standalone manner for you case ;)).
Hope it helps
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 26 avr. 2022 à 21:41, Romain Manni-Bucau <rm...@gmail.com> a
écrit :
> Hmm, you know what, from the size of the impl which mainly delegate to
> Charset.newEncoder()... I think we should just implement our own
> SizedOutputStreamWriter(Charset, OutputStream, int
> buffer)....even java.nio.channels.Channels#newWriter(java.nio.channels.WritableByteChannel,
> java.nio.charset.CharsetEncoder, int) workaround does not work cause the
> buffer size is a min for StreamEncoder so just doing our own looks way
> simpler overall.
> Would it help?
>
> That said I just checked and looks
> like org.apache.johnzon.core.JsonGeneratorFactoryImpl#createGenerator(java.io.OutputStream)
> has a bug to not fallback on a configured encoding - we do it for the
> reader but not generator which should reuse the
> org.apache.johnzon.encoding property , this is a nasty bug making us not
> symmetric - nor functional - with some API :(. Will try to have a look
> tomorrow if I get time. Guess we'll end up
> dropping org.apache.johnzon.core.JsonGeneratorImpl#JsonGeneratorImpl(java.io.OutputStream,
> org.apache.johnzon.core.BufferStrategy.BufferProvider<char[]>, boolean).
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le mar. 26 avr. 2022 à 19:55, David Blevins <da...@gmail.com> a
> écrit :
>
>> The critical reason for the switch from OutputStream to Writer is the
>> hardcoded 8k buffer inside OutputStreamWriter, which prevents our
>> optimization from working. We'd still have been writing up to 8k. I
>> switched to Writer to be in front of that buffer rather than behind it.
>>
>> I'd need to check on the character encoding issue you mention. In my
>> mind the original code and current code is trying to create a string of max
>> snippet length. If it doesn't do that, it's a bug.
>>
>>
>> -David
>>
>>
>> > On Apr 26, 2022, at 10:10 AM, Romain Manni-Bucau <rm...@gmail.com>
>> wrote:
>> >
>> > Hi David,
>> >
>> > Just reviewed the PR, if you can quickly go through the
>> questions/points i
>> > can follow up tomorrow to finish it if you don't have time anymore.
>> > Long story short i'm a bit "??" on the move from stream to writer and I
>> > don't see how it works, rest is about being picky so I could handle it
>> > after the merge.
>> >
>> > Romain Manni-Bucau
>> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
>> > <https://rmannibucau.metawerx.net/> | Old Blog
>> > <http://rmannibucau.wordpress.com> | Github <
>> https://github.com/rmannibucau> |
>> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>> > <
>> https://www.packtpub.com/application-development/java-ee-8-high-performance
>> >
>> >
>> >
>> > Le mar. 26 avr. 2022 à 19:02, David Blevins <da...@gmail.com> a
>> > écrit :
>> >
>> >> I really like it. I pushed a version of this that gets to the heart of
>> >> the idea. I need to move on to the messages themselves as I have a
>> hard
>> >> stop approaching me, but I am supportive if you want to keep refining
>> it.
>> >>
>> >> Ideally, we merge and you make any changes you feel are necessary and I
>> >> make second PR for rolling this out to the messages.
>> >>
>> >>
>> >> -David
>> >>
>> >>> On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <
>> rmannibucau@gmail.com>
>> >> wrote:
>> >>>
>> >>> Hey David,
>> >>>
>> >>> Just got an idea we can maybe refine to merge both points.
>> >>> I'll try to draft it high level (understand a Writer can be an
>> >> OutputStream
>> >>> or the opposite in your impl) so don't get too picky about the details
>> >>> please ;).
>> >>>
>> >>> High level the idea is to make JsonGeneratorFactoryImpl to be able to
>> >>> handle an outputstream or writer which would provide its buffer size.
>> >>>
>> >>> Here are the idea steps I can envision:
>> >>>
>> >>> 1. Create a new interface in johnzon-core `public interface
>> >>> MetadataProvider { int bufferSize(); }`
>> >>> 2. Create an OutputStream implementation of MetadataProvider - let's
>> call
>> >>> it MetadataProviderOutputStream for this list - if it helps it can be
>> a
>> >>> MetadataProviderSnippetOutputStream directly which would just be a
>> >> subclass
>> >>> of SnippetOutputStream, think it would make it simpler.
>> >>> 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where
>> we
>> >>> create the Snippet instance - which will basically test the class
>> name of
>> >>> the generator factory - no reflection there please to stay compatible
>> >> with
>> >>> all our downstream consumers without changes - and if it
>> >>> is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will
>> create a
>> >>> MetadataProviderOutputStream else a plain OutputStream and the feature
>> >> will
>> >>> be disabled (for now at least) - note that it can need a small
>> >> indirection
>> >>> like a MetadataProviderOutputStreamSupplier to not trigger the load of
>> >> the
>> >>> MetadataProvider on MapperBuilder class init/verification. This will
>> also
>> >>> use the snippet max size indeed. Here we want to ensure we can run
>> >> without
>> >>> johnzon-core as JSON-P impl.
>> >>> 4. Pass the supplier created at 3 to the Snippet class next to the
>> >>> generator factory
>> >>> 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to
>> test
>> >> if
>> >>> it is a MetadataProvider instance to handle it (small trick there is
>> to
>> >> try
>> >>> to cache the snippet buffer to keep reusing the buffer caching - and
>> only
>> >>> when needed):
>> >>>
>> >>> public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
>> >>> implements JsonGeneratorFactory {
>> >>> public static final String GENERATOR_BUFFER_LENGTH =
>> >>> "org.apache.johnzon.default-char-buffer-generator";
>> >>> public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
>> >>> Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
>> >>>
>> >>> static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
>> >>> JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
>> >>> BUFFER_STRATEGY
>> >>> );
>> >>> //key caching currently disabled
>> >>> private final boolean pretty;
>> >>> private final Buffer buffer;
>> >>> private volatile Buffer firstCustomBuffer;
>> >>>
>> >>> public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
>> >>> super(config, SUPPORTED_CONFIG_KEYS, null);
>> >>> this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
>> >>>
>> >>> final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
>> >>> DEFAULT_GENERATOR_BUFFER_LENGTH);
>> >>> if (bufferSize <= 0) {
>> >>> throw new IllegalArgumentException("buffer length must be
>> >>> greater than zero");
>> >>> }
>> >>> this.buffer = new
>> >>> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
>> >>> }
>> >>>
>> >>> @Override
>> >>> public JsonGenerator createGenerator(final Writer writer) {
>> >>> return new JsonGeneratorImpl(writer, buffer.provider, pretty);
>> >>> }
>> >>>
>> >>> @Override
>> >>> public JsonGenerator createGenerator(final OutputStream out) {
>> >>> if (MetadataProvider.class.isInstance(out)) { // mainly for
>> >> Snippet
>> >>> handling since generators don't have properties
>> >>> final int bufferSize =
>> >>> MetadataProvider.class.cast(out).bufferSize();
>> >>> if (buffer.size != bufferSize) {
>> >>> // most of the time it will be a single aside buffer so
>> >>> keep its factory to reuse the provider cache
>> >>> if (firstCustomBuffer == null) {
>> >>> synchronized (this) {
>> >>> if (firstCustomBuffer == null) {
>> >>> firstCustomBuffer = new
>> >>> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
>> >>> }
>> >>> }
>> >>> }
>> >>> Buffer customBuffer = firstCustomBuffer;
>> >>> if (customBuffer.size != bufferSize) { // don't use the
>> >>> cache, shouldn't occur often
>> >>> return new JsonGeneratorImpl(out,
>> >>> getBufferProvider().newCharProvider(bufferSize), pretty);
>> >>> }
>> >>> return new JsonGeneratorImpl(out, customBuffer.provider,
>> >>> pretty);
>> >>> } // else just reuse the original buffer since it matches
>> and
>> >>> leverages its cache
>> >>> }
>> >>> return new JsonGeneratorImpl(out, buffer.provider, pretty);
>> >>> }
>> >>>
>> >>> @Override
>> >>> public JsonGenerator createGenerator(final OutputStream out, final
>> >>> Charset charset) {
>> >>> return new JsonGeneratorImpl(out,charset, buffer.provider,
>> >> pretty);
>> >>> }
>> >>>
>> >>> @Override
>> >>> public Map<String, ?> getConfigInUse() {
>> >>> return Collections.unmodifiableMap(internalConfig);
>> >>> }
>> >>>
>> >>> private static final class Buffer {
>> >>> private final BufferStrategy.BufferProvider<char[]> provider;
>> >>> private final int size;
>> >>>
>> >>> private Buffer(final BufferStrategy.BufferProvider<char[]>
>> >>> bufferProvider, final int bufferSize) {
>> >>> this.provider = bufferProvider;
>> >>> this.size = bufferSize;
>> >>> }
>> >>> }
>> >>> }
>> >>>
>> >>> 6. you use the outputstream supplier in Snippet and done
>> >>>
>> >>> Wdyt?
>> >>>
>> >>> Romain Manni-Bucau
>> >>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>> >>> <https://rmannibucau.metawerx.net/> | Old Blog
>> >>> <http://rmannibucau.wordpress.com> | Github <
>> >> https://github.com/rmannibucau> |
>> >>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>> >>> <
>> >>
>> https://www.packtpub.com/application-development/java-ee-8-high-performance
>> >>>
>> >>>
>> >>>
>> >>> Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <
>> rmannibucau@gmail.com>
>> >> a
>> >>> écrit :
>> >>>
>> >>>>
>> >>>>
>> >>>> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com>
>> a
>> >>>> écrit :
>> >>>>
>> >>>>> Since we're now getting the snippet max length via the config,
>> could we
>> >>>>> construct the JsonGeneratorImpl directly and pass in a buffer of the
>> >>>>> correct size?
>> >>>>>
>> >>>>
>> >>>> From the JsonProvider we can - mapper must not depend on
>> johnzon-core -
>> >> at
>> >>>> the condition to promote the other properties too AND not do it if
>> user
>> >>>> provided a custom generator factory which would just be reused in
>> this
>> >>>> case, so not sure it solves the issue.
>> >>>>
>> >>>>
>> >>>>> -David
>> >>>>>
>> >>>>>
>> >>>>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <
>> >> rmannibucau@gmail.com>
>> >>>>> wrote:
>> >>>>>>
>> >>>>>> No worries, got it.
>> >>>>>>
>> >>>>>> One issue copying the generator is that it will still miss the
>> flush
>> >> for
>> >>>>>> all small objects and for all other cases the heuristic + a simple
>> >>>>>> truncation if needed would work with almost no other downsides than
>> >> not
>> >>>>>> being exact but goal was to limit the overflow/cpu/mem so all good
>> >> IMHO.
>> >>>>>>
>> >>>>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <
>> david.blevins@gmail.com>
>> >> a
>> >>>>>> écrit :
>> >>>>>>
>> >>>>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <
>> >> david.blevins@gmail.com>
>> >>>>>>> wrote:
>> >>>>>>>>
>> >>>>>>>> The trick with that is there is yet another buffer being held
>> >>>>> internally
>> >>>>>>> by ObjectOutputStream and it recreates the issue.
>> >>>>>>>
>> >>>>>>> Just read this again -- this was supposed to be
>> "OutputStreamWriter"
>> >>>>> not
>> >>>>>>> ObjectOutputStream. Too many years of EJB :)
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> -David
>> >>>>>>>
>> >>>>>>>
>> >>>>>
>> >>>>>
>> >>
>> >>
>>
>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hmm, you know what, from the size of the impl which mainly delegate to
Charset.newEncoder()... I think we should just implement our own
SizedOutputStreamWriter(Charset, OutputStream, int
buffer)....even
java.nio.channels.Channels#newWriter(java.nio.channels.WritableByteChannel,
java.nio.charset.CharsetEncoder, int) workaround does not work cause the
buffer size is a min for StreamEncoder so just doing our own looks way
simpler overall.
Would it help?
That said I just checked and looks
like org.apache.johnzon.core.JsonGeneratorFactoryImpl#createGenerator(java.io.OutputStream)
has a bug to not fallback on a configured encoding - we do it for the
reader but not generator which should reuse the
org.apache.johnzon.encoding property
, this is a nasty bug making us not symmetric - nor functional - with some
API :(. Will try to have a look tomorrow if I get time. Guess we'll end up
dropping org.apache.johnzon.core.JsonGeneratorImpl#JsonGeneratorImpl(java.io.OutputStream,
org.apache.johnzon.core.BufferStrategy.BufferProvider<char[]>, boolean).
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 26 avr. 2022 à 19:55, David Blevins <da...@gmail.com> a
écrit :
> The critical reason for the switch from OutputStream to Writer is the
> hardcoded 8k buffer inside OutputStreamWriter, which prevents our
> optimization from working. We'd still have been writing up to 8k. I
> switched to Writer to be in front of that buffer rather than behind it.
>
> I'd need to check on the character encoding issue you mention. In my mind
> the original code and current code is trying to create a string of max
> snippet length. If it doesn't do that, it's a bug.
>
>
> -David
>
>
> > On Apr 26, 2022, at 10:10 AM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Hi David,
> >
> > Just reviewed the PR, if you can quickly go through the questions/points
> i
> > can follow up tomorrow to finish it if you don't have time anymore.
> > Long story short i'm a bit "??" on the move from stream to writer and I
> > don't see how it works, rest is about being picky so I could handle it
> > after the merge.
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
> >
> >
> > Le mar. 26 avr. 2022 à 19:02, David Blevins <da...@gmail.com> a
> > écrit :
> >
> >> I really like it. I pushed a version of this that gets to the heart of
> >> the idea. I need to move on to the messages themselves as I have a hard
> >> stop approaching me, but I am supportive if you want to keep refining
> it.
> >>
> >> Ideally, we merge and you make any changes you feel are necessary and I
> >> make second PR for rolling this out to the messages.
> >>
> >>
> >> -David
> >>
> >>> On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <
> rmannibucau@gmail.com>
> >> wrote:
> >>>
> >>> Hey David,
> >>>
> >>> Just got an idea we can maybe refine to merge both points.
> >>> I'll try to draft it high level (understand a Writer can be an
> >> OutputStream
> >>> or the opposite in your impl) so don't get too picky about the details
> >>> please ;).
> >>>
> >>> High level the idea is to make JsonGeneratorFactoryImpl to be able to
> >>> handle an outputstream or writer which would provide its buffer size.
> >>>
> >>> Here are the idea steps I can envision:
> >>>
> >>> 1. Create a new interface in johnzon-core `public interface
> >>> MetadataProvider { int bufferSize(); }`
> >>> 2. Create an OutputStream implementation of MetadataProvider - let's
> call
> >>> it MetadataProviderOutputStream for this list - if it helps it can be a
> >>> MetadataProviderSnippetOutputStream directly which would just be a
> >> subclass
> >>> of SnippetOutputStream, think it would make it simpler.
> >>> 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where
> we
> >>> create the Snippet instance - which will basically test the class name
> of
> >>> the generator factory - no reflection there please to stay compatible
> >> with
> >>> all our downstream consumers without changes - and if it
> >>> is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will
> create a
> >>> MetadataProviderOutputStream else a plain OutputStream and the feature
> >> will
> >>> be disabled (for now at least) - note that it can need a small
> >> indirection
> >>> like a MetadataProviderOutputStreamSupplier to not trigger the load of
> >> the
> >>> MetadataProvider on MapperBuilder class init/verification. This will
> also
> >>> use the snippet max size indeed. Here we want to ensure we can run
> >> without
> >>> johnzon-core as JSON-P impl.
> >>> 4. Pass the supplier created at 3 to the Snippet class next to the
> >>> generator factory
> >>> 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to
> test
> >> if
> >>> it is a MetadataProvider instance to handle it (small trick there is to
> >> try
> >>> to cache the snippet buffer to keep reusing the buffer caching - and
> only
> >>> when needed):
> >>>
> >>> public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
> >>> implements JsonGeneratorFactory {
> >>> public static final String GENERATOR_BUFFER_LENGTH =
> >>> "org.apache.johnzon.default-char-buffer-generator";
> >>> public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
> >>> Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
> >>>
> >>> static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
> >>> JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
> >>> BUFFER_STRATEGY
> >>> );
> >>> //key caching currently disabled
> >>> private final boolean pretty;
> >>> private final Buffer buffer;
> >>> private volatile Buffer firstCustomBuffer;
> >>>
> >>> public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
> >>> super(config, SUPPORTED_CONFIG_KEYS, null);
> >>> this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
> >>>
> >>> final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
> >>> DEFAULT_GENERATOR_BUFFER_LENGTH);
> >>> if (bufferSize <= 0) {
> >>> throw new IllegalArgumentException("buffer length must be
> >>> greater than zero");
> >>> }
> >>> this.buffer = new
> >>> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> >>> }
> >>>
> >>> @Override
> >>> public JsonGenerator createGenerator(final Writer writer) {
> >>> return new JsonGeneratorImpl(writer, buffer.provider, pretty);
> >>> }
> >>>
> >>> @Override
> >>> public JsonGenerator createGenerator(final OutputStream out) {
> >>> if (MetadataProvider.class.isInstance(out)) { // mainly for
> >> Snippet
> >>> handling since generators don't have properties
> >>> final int bufferSize =
> >>> MetadataProvider.class.cast(out).bufferSize();
> >>> if (buffer.size != bufferSize) {
> >>> // most of the time it will be a single aside buffer so
> >>> keep its factory to reuse the provider cache
> >>> if (firstCustomBuffer == null) {
> >>> synchronized (this) {
> >>> if (firstCustomBuffer == null) {
> >>> firstCustomBuffer = new
> >>> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> >>> }
> >>> }
> >>> }
> >>> Buffer customBuffer = firstCustomBuffer;
> >>> if (customBuffer.size != bufferSize) { // don't use the
> >>> cache, shouldn't occur often
> >>> return new JsonGeneratorImpl(out,
> >>> getBufferProvider().newCharProvider(bufferSize), pretty);
> >>> }
> >>> return new JsonGeneratorImpl(out, customBuffer.provider,
> >>> pretty);
> >>> } // else just reuse the original buffer since it matches and
> >>> leverages its cache
> >>> }
> >>> return new JsonGeneratorImpl(out, buffer.provider, pretty);
> >>> }
> >>>
> >>> @Override
> >>> public JsonGenerator createGenerator(final OutputStream out, final
> >>> Charset charset) {
> >>> return new JsonGeneratorImpl(out,charset, buffer.provider,
> >> pretty);
> >>> }
> >>>
> >>> @Override
> >>> public Map<String, ?> getConfigInUse() {
> >>> return Collections.unmodifiableMap(internalConfig);
> >>> }
> >>>
> >>> private static final class Buffer {
> >>> private final BufferStrategy.BufferProvider<char[]> provider;
> >>> private final int size;
> >>>
> >>> private Buffer(final BufferStrategy.BufferProvider<char[]>
> >>> bufferProvider, final int bufferSize) {
> >>> this.provider = bufferProvider;
> >>> this.size = bufferSize;
> >>> }
> >>> }
> >>> }
> >>>
> >>> 6. you use the outputstream supplier in Snippet and done
> >>>
> >>> Wdyt?
> >>>
> >>> Romain Manni-Bucau
> >>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> >>> <https://rmannibucau.metawerx.net/> | Old Blog
> >>> <http://rmannibucau.wordpress.com> | Github <
> >> https://github.com/rmannibucau> |
> >>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> >>> <
> >>
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >>>
> >>>
> >>>
> >>> Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <
> rmannibucau@gmail.com>
> >> a
> >>> écrit :
> >>>
> >>>>
> >>>>
> >>>> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com>
> a
> >>>> écrit :
> >>>>
> >>>>> Since we're now getting the snippet max length via the config, could
> we
> >>>>> construct the JsonGeneratorImpl directly and pass in a buffer of the
> >>>>> correct size?
> >>>>>
> >>>>
> >>>> From the JsonProvider we can - mapper must not depend on johnzon-core
> -
> >> at
> >>>> the condition to promote the other properties too AND not do it if
> user
> >>>> provided a custom generator factory which would just be reused in this
> >>>> case, so not sure it solves the issue.
> >>>>
> >>>>
> >>>>> -David
> >>>>>
> >>>>>
> >>>>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <
> >> rmannibucau@gmail.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> No worries, got it.
> >>>>>>
> >>>>>> One issue copying the generator is that it will still miss the flush
> >> for
> >>>>>> all small objects and for all other cases the heuristic + a simple
> >>>>>> truncation if needed would work with almost no other downsides than
> >> not
> >>>>>> being exact but goal was to limit the overflow/cpu/mem so all good
> >> IMHO.
> >>>>>>
> >>>>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <
> david.blevins@gmail.com>
> >> a
> >>>>>> écrit :
> >>>>>>
> >>>>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <
> >> david.blevins@gmail.com>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> The trick with that is there is yet another buffer being held
> >>>>> internally
> >>>>>>> by ObjectOutputStream and it recreates the issue.
> >>>>>>>
> >>>>>>> Just read this again -- this was supposed to be
> "OutputStreamWriter"
> >>>>> not
> >>>>>>> ObjectOutputStream. Too many years of EJB :)
> >>>>>>>
> >>>>>>>
> >>>>>>> -David
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>
> >>
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
The critical reason for the switch from OutputStream to Writer is the hardcoded 8k buffer inside OutputStreamWriter, which prevents our optimization from working. We'd still have been writing up to 8k. I switched to Writer to be in front of that buffer rather than behind it.
I'd need to check on the character encoding issue you mention. In my mind the original code and current code is trying to create a string of max snippet length. If it doesn't do that, it's a bug.
-David
> On Apr 26, 2022, at 10:10 AM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Hi David,
>
> Just reviewed the PR, if you can quickly go through the questions/points i
> can follow up tomorrow to finish it if you don't have time anymore.
> Long story short i'm a bit "??" on the move from stream to writer and I
> don't see how it works, rest is about being picky so I could handle it
> after the merge.
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le mar. 26 avr. 2022 à 19:02, David Blevins <da...@gmail.com> a
> écrit :
>
>> I really like it. I pushed a version of this that gets to the heart of
>> the idea. I need to move on to the messages themselves as I have a hard
>> stop approaching me, but I am supportive if you want to keep refining it.
>>
>> Ideally, we merge and you make any changes you feel are necessary and I
>> make second PR for rolling this out to the messages.
>>
>>
>> -David
>>
>>> On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <rm...@gmail.com>
>> wrote:
>>>
>>> Hey David,
>>>
>>> Just got an idea we can maybe refine to merge both points.
>>> I'll try to draft it high level (understand a Writer can be an
>> OutputStream
>>> or the opposite in your impl) so don't get too picky about the details
>>> please ;).
>>>
>>> High level the idea is to make JsonGeneratorFactoryImpl to be able to
>>> handle an outputstream or writer which would provide its buffer size.
>>>
>>> Here are the idea steps I can envision:
>>>
>>> 1. Create a new interface in johnzon-core `public interface
>>> MetadataProvider { int bufferSize(); }`
>>> 2. Create an OutputStream implementation of MetadataProvider - let's call
>>> it MetadataProviderOutputStream for this list - if it helps it can be a
>>> MetadataProviderSnippetOutputStream directly which would just be a
>> subclass
>>> of SnippetOutputStream, think it would make it simpler.
>>> 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where we
>>> create the Snippet instance - which will basically test the class name of
>>> the generator factory - no reflection there please to stay compatible
>> with
>>> all our downstream consumers without changes - and if it
>>> is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will create a
>>> MetadataProviderOutputStream else a plain OutputStream and the feature
>> will
>>> be disabled (for now at least) - note that it can need a small
>> indirection
>>> like a MetadataProviderOutputStreamSupplier to not trigger the load of
>> the
>>> MetadataProvider on MapperBuilder class init/verification. This will also
>>> use the snippet max size indeed. Here we want to ensure we can run
>> without
>>> johnzon-core as JSON-P impl.
>>> 4. Pass the supplier created at 3 to the Snippet class next to the
>>> generator factory
>>> 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to test
>> if
>>> it is a MetadataProvider instance to handle it (small trick there is to
>> try
>>> to cache the snippet buffer to keep reusing the buffer caching - and only
>>> when needed):
>>>
>>> public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
>>> implements JsonGeneratorFactory {
>>> public static final String GENERATOR_BUFFER_LENGTH =
>>> "org.apache.johnzon.default-char-buffer-generator";
>>> public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
>>> Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
>>>
>>> static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
>>> JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
>>> BUFFER_STRATEGY
>>> );
>>> //key caching currently disabled
>>> private final boolean pretty;
>>> private final Buffer buffer;
>>> private volatile Buffer firstCustomBuffer;
>>>
>>> public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
>>> super(config, SUPPORTED_CONFIG_KEYS, null);
>>> this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
>>>
>>> final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
>>> DEFAULT_GENERATOR_BUFFER_LENGTH);
>>> if (bufferSize <= 0) {
>>> throw new IllegalArgumentException("buffer length must be
>>> greater than zero");
>>> }
>>> this.buffer = new
>>> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
>>> }
>>>
>>> @Override
>>> public JsonGenerator createGenerator(final Writer writer) {
>>> return new JsonGeneratorImpl(writer, buffer.provider, pretty);
>>> }
>>>
>>> @Override
>>> public JsonGenerator createGenerator(final OutputStream out) {
>>> if (MetadataProvider.class.isInstance(out)) { // mainly for
>> Snippet
>>> handling since generators don't have properties
>>> final int bufferSize =
>>> MetadataProvider.class.cast(out).bufferSize();
>>> if (buffer.size != bufferSize) {
>>> // most of the time it will be a single aside buffer so
>>> keep its factory to reuse the provider cache
>>> if (firstCustomBuffer == null) {
>>> synchronized (this) {
>>> if (firstCustomBuffer == null) {
>>> firstCustomBuffer = new
>>> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
>>> }
>>> }
>>> }
>>> Buffer customBuffer = firstCustomBuffer;
>>> if (customBuffer.size != bufferSize) { // don't use the
>>> cache, shouldn't occur often
>>> return new JsonGeneratorImpl(out,
>>> getBufferProvider().newCharProvider(bufferSize), pretty);
>>> }
>>> return new JsonGeneratorImpl(out, customBuffer.provider,
>>> pretty);
>>> } // else just reuse the original buffer since it matches and
>>> leverages its cache
>>> }
>>> return new JsonGeneratorImpl(out, buffer.provider, pretty);
>>> }
>>>
>>> @Override
>>> public JsonGenerator createGenerator(final OutputStream out, final
>>> Charset charset) {
>>> return new JsonGeneratorImpl(out,charset, buffer.provider,
>> pretty);
>>> }
>>>
>>> @Override
>>> public Map<String, ?> getConfigInUse() {
>>> return Collections.unmodifiableMap(internalConfig);
>>> }
>>>
>>> private static final class Buffer {
>>> private final BufferStrategy.BufferProvider<char[]> provider;
>>> private final int size;
>>>
>>> private Buffer(final BufferStrategy.BufferProvider<char[]>
>>> bufferProvider, final int bufferSize) {
>>> this.provider = bufferProvider;
>>> this.size = bufferSize;
>>> }
>>> }
>>> }
>>>
>>> 6. you use the outputstream supplier in Snippet and done
>>>
>>> Wdyt?
>>>
>>> Romain Manni-Bucau
>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>> <http://rmannibucau.wordpress.com> | Github <
>> https://github.com/rmannibucau> |
>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>>> <
>> https://www.packtpub.com/application-development/java-ee-8-high-performance
>>>
>>>
>>>
>>> Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <rm...@gmail.com>
>> a
>>> écrit :
>>>
>>>>
>>>>
>>>> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
>>>> écrit :
>>>>
>>>>> Since we're now getting the snippet max length via the config, could we
>>>>> construct the JsonGeneratorImpl directly and pass in a buffer of the
>>>>> correct size?
>>>>>
>>>>
>>>> From the JsonProvider we can - mapper must not depend on johnzon-core -
>> at
>>>> the condition to promote the other properties too AND not do it if user
>>>> provided a custom generator factory which would just be reused in this
>>>> case, so not sure it solves the issue.
>>>>
>>>>
>>>>> -David
>>>>>
>>>>>
>>>>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <
>> rmannibucau@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> No worries, got it.
>>>>>>
>>>>>> One issue copying the generator is that it will still miss the flush
>> for
>>>>>> all small objects and for all other cases the heuristic + a simple
>>>>>> truncation if needed would work with almost no other downsides than
>> not
>>>>>> being exact but goal was to limit the overflow/cpu/mem so all good
>> IMHO.
>>>>>>
>>>>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com>
>> a
>>>>>> écrit :
>>>>>>
>>>>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <
>> david.blevins@gmail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> The trick with that is there is yet another buffer being held
>>>>> internally
>>>>>>> by ObjectOutputStream and it recreates the issue.
>>>>>>>
>>>>>>> Just read this again -- this was supposed to be "OutputStreamWriter"
>>>>> not
>>>>>>> ObjectOutputStream. Too many years of EJB :)
>>>>>>>
>>>>>>>
>>>>>>> -David
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>
>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi David,
Just reviewed the PR, if you can quickly go through the questions/points i
can follow up tomorrow to finish it if you don't have time anymore.
Long story short i'm a bit "??" on the move from stream to writer and I
don't see how it works, rest is about being picky so I could handle it
after the merge.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 26 avr. 2022 à 19:02, David Blevins <da...@gmail.com> a
écrit :
> I really like it. I pushed a version of this that gets to the heart of
> the idea. I need to move on to the messages themselves as I have a hard
> stop approaching me, but I am supportive if you want to keep refining it.
>
> Ideally, we merge and you make any changes you feel are necessary and I
> make second PR for rolling this out to the messages.
>
>
> -David
>
> > On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Hey David,
> >
> > Just got an idea we can maybe refine to merge both points.
> > I'll try to draft it high level (understand a Writer can be an
> OutputStream
> > or the opposite in your impl) so don't get too picky about the details
> > please ;).
> >
> > High level the idea is to make JsonGeneratorFactoryImpl to be able to
> > handle an outputstream or writer which would provide its buffer size.
> >
> > Here are the idea steps I can envision:
> >
> > 1. Create a new interface in johnzon-core `public interface
> > MetadataProvider { int bufferSize(); }`
> > 2. Create an OutputStream implementation of MetadataProvider - let's call
> > it MetadataProviderOutputStream for this list - if it helps it can be a
> > MetadataProviderSnippetOutputStream directly which would just be a
> subclass
> > of SnippetOutputStream, think it would make it simpler.
> > 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where we
> > create the Snippet instance - which will basically test the class name of
> > the generator factory - no reflection there please to stay compatible
> with
> > all our downstream consumers without changes - and if it
> > is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will create a
> > MetadataProviderOutputStream else a plain OutputStream and the feature
> will
> > be disabled (for now at least) - note that it can need a small
> indirection
> > like a MetadataProviderOutputStreamSupplier to not trigger the load of
> the
> > MetadataProvider on MapperBuilder class init/verification. This will also
> > use the snippet max size indeed. Here we want to ensure we can run
> without
> > johnzon-core as JSON-P impl.
> > 4. Pass the supplier created at 3 to the Snippet class next to the
> > generator factory
> > 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to test
> if
> > it is a MetadataProvider instance to handle it (small trick there is to
> try
> > to cache the snippet buffer to keep reusing the buffer caching - and only
> > when needed):
> >
> > public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
> > implements JsonGeneratorFactory {
> > public static final String GENERATOR_BUFFER_LENGTH =
> > "org.apache.johnzon.default-char-buffer-generator";
> > public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
> > Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
> >
> > static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
> > JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
> > BUFFER_STRATEGY
> > );
> > //key caching currently disabled
> > private final boolean pretty;
> > private final Buffer buffer;
> > private volatile Buffer firstCustomBuffer;
> >
> > public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
> > super(config, SUPPORTED_CONFIG_KEYS, null);
> > this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
> >
> > final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
> > DEFAULT_GENERATOR_BUFFER_LENGTH);
> > if (bufferSize <= 0) {
> > throw new IllegalArgumentException("buffer length must be
> > greater than zero");
> > }
> > this.buffer = new
> > Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> > }
> >
> > @Override
> > public JsonGenerator createGenerator(final Writer writer) {
> > return new JsonGeneratorImpl(writer, buffer.provider, pretty);
> > }
> >
> > @Override
> > public JsonGenerator createGenerator(final OutputStream out) {
> > if (MetadataProvider.class.isInstance(out)) { // mainly for
> Snippet
> > handling since generators don't have properties
> > final int bufferSize =
> > MetadataProvider.class.cast(out).bufferSize();
> > if (buffer.size != bufferSize) {
> > // most of the time it will be a single aside buffer so
> > keep its factory to reuse the provider cache
> > if (firstCustomBuffer == null) {
> > synchronized (this) {
> > if (firstCustomBuffer == null) {
> > firstCustomBuffer = new
> > Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> > }
> > }
> > }
> > Buffer customBuffer = firstCustomBuffer;
> > if (customBuffer.size != bufferSize) { // don't use the
> > cache, shouldn't occur often
> > return new JsonGeneratorImpl(out,
> > getBufferProvider().newCharProvider(bufferSize), pretty);
> > }
> > return new JsonGeneratorImpl(out, customBuffer.provider,
> > pretty);
> > } // else just reuse the original buffer since it matches and
> > leverages its cache
> > }
> > return new JsonGeneratorImpl(out, buffer.provider, pretty);
> > }
> >
> > @Override
> > public JsonGenerator createGenerator(final OutputStream out, final
> > Charset charset) {
> > return new JsonGeneratorImpl(out,charset, buffer.provider,
> pretty);
> > }
> >
> > @Override
> > public Map<String, ?> getConfigInUse() {
> > return Collections.unmodifiableMap(internalConfig);
> > }
> >
> > private static final class Buffer {
> > private final BufferStrategy.BufferProvider<char[]> provider;
> > private final int size;
> >
> > private Buffer(final BufferStrategy.BufferProvider<char[]>
> > bufferProvider, final int bufferSize) {
> > this.provider = bufferProvider;
> > this.size = bufferSize;
> > }
> > }
> > }
> >
> > 6. you use the outputstream supplier in Snippet and done
> >
> > Wdyt?
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
> >
> >
> > Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <rm...@gmail.com>
> a
> > écrit :
> >
> >>
> >>
> >> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
> >> écrit :
> >>
> >>> Since we're now getting the snippet max length via the config, could we
> >>> construct the JsonGeneratorImpl directly and pass in a buffer of the
> >>> correct size?
> >>>
> >>
> >> From the JsonProvider we can - mapper must not depend on johnzon-core -
> at
> >> the condition to promote the other properties too AND not do it if user
> >> provided a custom generator factory which would just be reused in this
> >> case, so not sure it solves the issue.
> >>
> >>
> >>> -David
> >>>
> >>>
> >>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <
> rmannibucau@gmail.com>
> >>> wrote:
> >>>>
> >>>> No worries, got it.
> >>>>
> >>>> One issue copying the generator is that it will still miss the flush
> for
> >>>> all small objects and for all other cases the heuristic + a simple
> >>>> truncation if needed would work with almost no other downsides than
> not
> >>>> being exact but goal was to limit the overflow/cpu/mem so all good
> IMHO.
> >>>>
> >>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com>
> a
> >>>> écrit :
> >>>>
> >>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <
> david.blevins@gmail.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> The trick with that is there is yet another buffer being held
> >>> internally
> >>>>> by ObjectOutputStream and it recreates the issue.
> >>>>>
> >>>>> Just read this again -- this was supposed to be "OutputStreamWriter"
> >>> not
> >>>>> ObjectOutputStream. Too many years of EJB :)
> >>>>>
> >>>>>
> >>>>> -David
> >>>>>
> >>>>>
> >>>
> >>>
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
I really like it. I pushed a version of this that gets to the heart of the idea. I need to move on to the messages themselves as I have a hard stop approaching me, but I am supportive if you want to keep refining it.
Ideally, we merge and you make any changes you feel are necessary and I make second PR for rolling this out to the messages.
-David
> On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Hey David,
>
> Just got an idea we can maybe refine to merge both points.
> I'll try to draft it high level (understand a Writer can be an OutputStream
> or the opposite in your impl) so don't get too picky about the details
> please ;).
>
> High level the idea is to make JsonGeneratorFactoryImpl to be able to
> handle an outputstream or writer which would provide its buffer size.
>
> Here are the idea steps I can envision:
>
> 1. Create a new interface in johnzon-core `public interface
> MetadataProvider { int bufferSize(); }`
> 2. Create an OutputStream implementation of MetadataProvider - let's call
> it MetadataProviderOutputStream for this list - if it helps it can be a
> MetadataProviderSnippetOutputStream directly which would just be a subclass
> of SnippetOutputStream, think it would make it simpler.
> 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where we
> create the Snippet instance - which will basically test the class name of
> the generator factory - no reflection there please to stay compatible with
> all our downstream consumers without changes - and if it
> is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will create a
> MetadataProviderOutputStream else a plain OutputStream and the feature will
> be disabled (for now at least) - note that it can need a small indirection
> like a MetadataProviderOutputStreamSupplier to not trigger the load of the
> MetadataProvider on MapperBuilder class init/verification. This will also
> use the snippet max size indeed. Here we want to ensure we can run without
> johnzon-core as JSON-P impl.
> 4. Pass the supplier created at 3 to the Snippet class next to the
> generator factory
> 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to test if
> it is a MetadataProvider instance to handle it (small trick there is to try
> to cache the snippet buffer to keep reusing the buffer caching - and only
> when needed):
>
> public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
> implements JsonGeneratorFactory {
> public static final String GENERATOR_BUFFER_LENGTH =
> "org.apache.johnzon.default-char-buffer-generator";
> public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
> Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
>
> static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
> JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
> BUFFER_STRATEGY
> );
> //key caching currently disabled
> private final boolean pretty;
> private final Buffer buffer;
> private volatile Buffer firstCustomBuffer;
>
> public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
> super(config, SUPPORTED_CONFIG_KEYS, null);
> this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
>
> final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
> DEFAULT_GENERATOR_BUFFER_LENGTH);
> if (bufferSize <= 0) {
> throw new IllegalArgumentException("buffer length must be
> greater than zero");
> }
> this.buffer = new
> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> }
>
> @Override
> public JsonGenerator createGenerator(final Writer writer) {
> return new JsonGeneratorImpl(writer, buffer.provider, pretty);
> }
>
> @Override
> public JsonGenerator createGenerator(final OutputStream out) {
> if (MetadataProvider.class.isInstance(out)) { // mainly for Snippet
> handling since generators don't have properties
> final int bufferSize =
> MetadataProvider.class.cast(out).bufferSize();
> if (buffer.size != bufferSize) {
> // most of the time it will be a single aside buffer so
> keep its factory to reuse the provider cache
> if (firstCustomBuffer == null) {
> synchronized (this) {
> if (firstCustomBuffer == null) {
> firstCustomBuffer = new
> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> }
> }
> }
> Buffer customBuffer = firstCustomBuffer;
> if (customBuffer.size != bufferSize) { // don't use the
> cache, shouldn't occur often
> return new JsonGeneratorImpl(out,
> getBufferProvider().newCharProvider(bufferSize), pretty);
> }
> return new JsonGeneratorImpl(out, customBuffer.provider,
> pretty);
> } // else just reuse the original buffer since it matches and
> leverages its cache
> }
> return new JsonGeneratorImpl(out, buffer.provider, pretty);
> }
>
> @Override
> public JsonGenerator createGenerator(final OutputStream out, final
> Charset charset) {
> return new JsonGeneratorImpl(out,charset, buffer.provider, pretty);
> }
>
> @Override
> public Map<String, ?> getConfigInUse() {
> return Collections.unmodifiableMap(internalConfig);
> }
>
> private static final class Buffer {
> private final BufferStrategy.BufferProvider<char[]> provider;
> private final int size;
>
> private Buffer(final BufferStrategy.BufferProvider<char[]>
> bufferProvider, final int bufferSize) {
> this.provider = bufferProvider;
> this.size = bufferSize;
> }
> }
> }
>
> 6. you use the outputstream supplier in Snippet and done
>
> Wdyt?
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <rm...@gmail.com> a
> écrit :
>
>>
>>
>> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
>> écrit :
>>
>>> Since we're now getting the snippet max length via the config, could we
>>> construct the JsonGeneratorImpl directly and pass in a buffer of the
>>> correct size?
>>>
>>
>> From the JsonProvider we can - mapper must not depend on johnzon-core - at
>> the condition to promote the other properties too AND not do it if user
>> provided a custom generator factory which would just be reused in this
>> case, so not sure it solves the issue.
>>
>>
>>> -David
>>>
>>>
>>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <rm...@gmail.com>
>>> wrote:
>>>>
>>>> No worries, got it.
>>>>
>>>> One issue copying the generator is that it will still miss the flush for
>>>> all small objects and for all other cases the heuristic + a simple
>>>> truncation if needed would work with almost no other downsides than not
>>>> being exact but goal was to limit the overflow/cpu/mem so all good IMHO.
>>>>
>>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com> a
>>>> écrit :
>>>>
>>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> The trick with that is there is yet another buffer being held
>>> internally
>>>>> by ObjectOutputStream and it recreates the issue.
>>>>>
>>>>> Just read this again -- this was supposed to be "OutputStreamWriter"
>>> not
>>>>> ObjectOutputStream. Too many years of EJB :)
>>>>>
>>>>>
>>>>> -David
>>>>>
>>>>>
>>>
>>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Code is way neater so it is a good move forward but it still does a lot of
buffer flushing and I think using a dedicated buffer or using the size
heuristic + tuncation if needed would be faster but would need to measure
it to be 100% sure.
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 26 avr. 2022 à 09:36, David Blevins <da...@gmail.com> a
écrit :
> Also had an idea as I was trying to go to bed. The thought occurred to me
> that we only need to call flush before calling
> snippetOutputStream.terminate(). So I reworked the code to do that. We
> also had some calls to terminate() that we didn't need because they were
> already getting called in the loops that proceeded them.
>
> I'll read what you wrote below, but can you take a look and let me know
> what you think?
>
>
> -David
>
>
> > On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Hey David,
> >
> > Just got an idea we can maybe refine to merge both points.
> > I'll try to draft it high level (understand a Writer can be an
> OutputStream
> > or the opposite in your impl) so don't get too picky about the details
> > please ;).
> >
> > High level the idea is to make JsonGeneratorFactoryImpl to be able to
> > handle an outputstream or writer which would provide its buffer size.
> >
> > Here are the idea steps I can envision:
> >
> > 1. Create a new interface in johnzon-core `public interface
> > MetadataProvider { int bufferSize(); }`
> > 2. Create an OutputStream implementation of MetadataProvider - let's call
> > it MetadataProviderOutputStream for this list - if it helps it can be a
> > MetadataProviderSnippetOutputStream directly which would just be a
> subclass
> > of SnippetOutputStream, think it would make it simpler.
> > 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where we
> > create the Snippet instance - which will basically test the class name of
> > the generator factory - no reflection there please to stay compatible
> with
> > all our downstream consumers without changes - and if it
> > is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will create a
> > MetadataProviderOutputStream else a plain OutputStream and the feature
> will
> > be disabled (for now at least) - note that it can need a small
> indirection
> > like a MetadataProviderOutputStreamSupplier to not trigger the load of
> the
> > MetadataProvider on MapperBuilder class init/verification. This will also
> > use the snippet max size indeed. Here we want to ensure we can run
> without
> > johnzon-core as JSON-P impl.
> > 4. Pass the supplier created at 3 to the Snippet class next to the
> > generator factory
> > 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to test
> if
> > it is a MetadataProvider instance to handle it (small trick there is to
> try
> > to cache the snippet buffer to keep reusing the buffer caching - and only
> > when needed):
> >
> > public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
> > implements JsonGeneratorFactory {
> > public static final String GENERATOR_BUFFER_LENGTH =
> > "org.apache.johnzon.default-char-buffer-generator";
> > public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
> > Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
> >
> > static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
> > JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
> > BUFFER_STRATEGY
> > );
> > //key caching currently disabled
> > private final boolean pretty;
> > private final Buffer buffer;
> > private volatile Buffer firstCustomBuffer;
> >
> > public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
> > super(config, SUPPORTED_CONFIG_KEYS, null);
> > this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
> >
> > final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
> > DEFAULT_GENERATOR_BUFFER_LENGTH);
> > if (bufferSize <= 0) {
> > throw new IllegalArgumentException("buffer length must be
> > greater than zero");
> > }
> > this.buffer = new
> > Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> > }
> >
> > @Override
> > public JsonGenerator createGenerator(final Writer writer) {
> > return new JsonGeneratorImpl(writer, buffer.provider, pretty);
> > }
> >
> > @Override
> > public JsonGenerator createGenerator(final OutputStream out) {
> > if (MetadataProvider.class.isInstance(out)) { // mainly for
> Snippet
> > handling since generators don't have properties
> > final int bufferSize =
> > MetadataProvider.class.cast(out).bufferSize();
> > if (buffer.size != bufferSize) {
> > // most of the time it will be a single aside buffer so
> > keep its factory to reuse the provider cache
> > if (firstCustomBuffer == null) {
> > synchronized (this) {
> > if (firstCustomBuffer == null) {
> > firstCustomBuffer = new
> > Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> > }
> > }
> > }
> > Buffer customBuffer = firstCustomBuffer;
> > if (customBuffer.size != bufferSize) { // don't use the
> > cache, shouldn't occur often
> > return new JsonGeneratorImpl(out,
> > getBufferProvider().newCharProvider(bufferSize), pretty);
> > }
> > return new JsonGeneratorImpl(out, customBuffer.provider,
> > pretty);
> > } // else just reuse the original buffer since it matches and
> > leverages its cache
> > }
> > return new JsonGeneratorImpl(out, buffer.provider, pretty);
> > }
> >
> > @Override
> > public JsonGenerator createGenerator(final OutputStream out, final
> > Charset charset) {
> > return new JsonGeneratorImpl(out,charset, buffer.provider,
> pretty);
> > }
> >
> > @Override
> > public Map<String, ?> getConfigInUse() {
> > return Collections.unmodifiableMap(internalConfig);
> > }
> >
> > private static final class Buffer {
> > private final BufferStrategy.BufferProvider<char[]> provider;
> > private final int size;
> >
> > private Buffer(final BufferStrategy.BufferProvider<char[]>
> > bufferProvider, final int bufferSize) {
> > this.provider = bufferProvider;
> > this.size = bufferSize;
> > }
> > }
> > }
> >
> > 6. you use the outputstream supplier in Snippet and done
> >
> > Wdyt?
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
> >
> >
> > Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <rm...@gmail.com>
> a
> > écrit :
> >
> >>
> >>
> >> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
> >> écrit :
> >>
> >>> Since we're now getting the snippet max length via the config, could we
> >>> construct the JsonGeneratorImpl directly and pass in a buffer of the
> >>> correct size?
> >>>
> >>
> >> From the JsonProvider we can - mapper must not depend on johnzon-core -
> at
> >> the condition to promote the other properties too AND not do it if user
> >> provided a custom generator factory which would just be reused in this
> >> case, so not sure it solves the issue.
> >>
> >>
> >>> -David
> >>>
> >>>
> >>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <
> rmannibucau@gmail.com>
> >>> wrote:
> >>>>
> >>>> No worries, got it.
> >>>>
> >>>> One issue copying the generator is that it will still miss the flush
> for
> >>>> all small objects and for all other cases the heuristic + a simple
> >>>> truncation if needed would work with almost no other downsides than
> not
> >>>> being exact but goal was to limit the overflow/cpu/mem so all good
> IMHO.
> >>>>
> >>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com>
> a
> >>>> écrit :
> >>>>
> >>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <
> david.blevins@gmail.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> The trick with that is there is yet another buffer being held
> >>> internally
> >>>>> by ObjectOutputStream and it recreates the issue.
> >>>>>
> >>>>> Just read this again -- this was supposed to be "OutputStreamWriter"
> >>> not
> >>>>> ObjectOutputStream. Too many years of EJB :)
> >>>>>
> >>>>>
> >>>>> -David
> >>>>>
> >>>>>
> >>>
> >>>
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
Also had an idea as I was trying to go to bed. The thought occurred to me that we only need to call flush before calling snippetOutputStream.terminate(). So I reworked the code to do that. We also had some calls to terminate() that we didn't need because they were already getting called in the loops that proceeded them.
I'll read what you wrote below, but can you take a look and let me know what you think?
-David
> On Apr 26, 2022, at 12:02 AM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Hey David,
>
> Just got an idea we can maybe refine to merge both points.
> I'll try to draft it high level (understand a Writer can be an OutputStream
> or the opposite in your impl) so don't get too picky about the details
> please ;).
>
> High level the idea is to make JsonGeneratorFactoryImpl to be able to
> handle an outputstream or writer which would provide its buffer size.
>
> Here are the idea steps I can envision:
>
> 1. Create a new interface in johnzon-core `public interface
> MetadataProvider { int bufferSize(); }`
> 2. Create an OutputStream implementation of MetadataProvider - let's call
> it MetadataProviderOutputStream for this list - if it helps it can be a
> MetadataProviderSnippetOutputStream directly which would just be a subclass
> of SnippetOutputStream, think it would make it simpler.
> 3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where we
> create the Snippet instance - which will basically test the class name of
> the generator factory - no reflection there please to stay compatible with
> all our downstream consumers without changes - and if it
> is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will create a
> MetadataProviderOutputStream else a plain OutputStream and the feature will
> be disabled (for now at least) - note that it can need a small indirection
> like a MetadataProviderOutputStreamSupplier to not trigger the load of the
> MetadataProvider on MapperBuilder class init/verification. This will also
> use the snippet max size indeed. Here we want to ensure we can run without
> johnzon-core as JSON-P impl.
> 4. Pass the supplier created at 3 to the Snippet class next to the
> generator factory
> 5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to test if
> it is a MetadataProvider instance to handle it (small trick there is to try
> to cache the snippet buffer to keep reusing the buffer caching - and only
> when needed):
>
> public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
> implements JsonGeneratorFactory {
> public static final String GENERATOR_BUFFER_LENGTH =
> "org.apache.johnzon.default-char-buffer-generator";
> public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
> Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
>
> static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
> JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
> BUFFER_STRATEGY
> );
> //key caching currently disabled
> private final boolean pretty;
> private final Buffer buffer;
> private volatile Buffer firstCustomBuffer;
>
> public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
> super(config, SUPPORTED_CONFIG_KEYS, null);
> this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
>
> final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
> DEFAULT_GENERATOR_BUFFER_LENGTH);
> if (bufferSize <= 0) {
> throw new IllegalArgumentException("buffer length must be
> greater than zero");
> }
> this.buffer = new
> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> }
>
> @Override
> public JsonGenerator createGenerator(final Writer writer) {
> return new JsonGeneratorImpl(writer, buffer.provider, pretty);
> }
>
> @Override
> public JsonGenerator createGenerator(final OutputStream out) {
> if (MetadataProvider.class.isInstance(out)) { // mainly for Snippet
> handling since generators don't have properties
> final int bufferSize =
> MetadataProvider.class.cast(out).bufferSize();
> if (buffer.size != bufferSize) {
> // most of the time it will be a single aside buffer so
> keep its factory to reuse the provider cache
> if (firstCustomBuffer == null) {
> synchronized (this) {
> if (firstCustomBuffer == null) {
> firstCustomBuffer = new
> Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
> }
> }
> }
> Buffer customBuffer = firstCustomBuffer;
> if (customBuffer.size != bufferSize) { // don't use the
> cache, shouldn't occur often
> return new JsonGeneratorImpl(out,
> getBufferProvider().newCharProvider(bufferSize), pretty);
> }
> return new JsonGeneratorImpl(out, customBuffer.provider,
> pretty);
> } // else just reuse the original buffer since it matches and
> leverages its cache
> }
> return new JsonGeneratorImpl(out, buffer.provider, pretty);
> }
>
> @Override
> public JsonGenerator createGenerator(final OutputStream out, final
> Charset charset) {
> return new JsonGeneratorImpl(out,charset, buffer.provider, pretty);
> }
>
> @Override
> public Map<String, ?> getConfigInUse() {
> return Collections.unmodifiableMap(internalConfig);
> }
>
> private static final class Buffer {
> private final BufferStrategy.BufferProvider<char[]> provider;
> private final int size;
>
> private Buffer(final BufferStrategy.BufferProvider<char[]>
> bufferProvider, final int bufferSize) {
> this.provider = bufferProvider;
> this.size = bufferSize;
> }
> }
> }
>
> 6. you use the outputstream supplier in Snippet and done
>
> Wdyt?
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <rm...@gmail.com> a
> écrit :
>
>>
>>
>> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
>> écrit :
>>
>>> Since we're now getting the snippet max length via the config, could we
>>> construct the JsonGeneratorImpl directly and pass in a buffer of the
>>> correct size?
>>>
>>
>> From the JsonProvider we can - mapper must not depend on johnzon-core - at
>> the condition to promote the other properties too AND not do it if user
>> provided a custom generator factory which would just be reused in this
>> case, so not sure it solves the issue.
>>
>>
>>> -David
>>>
>>>
>>>> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <rm...@gmail.com>
>>> wrote:
>>>>
>>>> No worries, got it.
>>>>
>>>> One issue copying the generator is that it will still miss the flush for
>>>> all small objects and for all other cases the heuristic + a simple
>>>> truncation if needed would work with almost no other downsides than not
>>>> being exact but goal was to limit the overflow/cpu/mem so all good IMHO.
>>>>
>>>> Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com> a
>>>> écrit :
>>>>
>>>>>> On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> The trick with that is there is yet another buffer being held
>>> internally
>>>>> by ObjectOutputStream and it recreates the issue.
>>>>>
>>>>> Just read this again -- this was supposed to be "OutputStreamWriter"
>>> not
>>>>> ObjectOutputStream. Too many years of EJB :)
>>>>>
>>>>>
>>>>> -David
>>>>>
>>>>>
>>>
>>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hey David,
Just got an idea we can maybe refine to merge both points.
I'll try to draft it high level (understand a Writer can be an OutputStream
or the opposite in your impl) so don't get too picky about the details
please ;).
High level the idea is to make JsonGeneratorFactoryImpl to be able to
handle an outputstream or writer which would provide its buffer size.
Here are the idea steps I can envision:
1. Create a new interface in johnzon-core `public interface
MetadataProvider { int bufferSize(); }`
2. Create an OutputStream implementation of MetadataProvider - let's call
it MetadataProviderOutputStream for this list - if it helps it can be a
MetadataProviderSnippetOutputStream directly which would just be a subclass
of SnippetOutputStream, think it would make it simpler.
3. Create a Supplier<OutputStream> (factory) in MapperBuilder - where we
create the Snippet instance - which will basically test the class name of
the generator factory - no reflection there please to stay compatible with
all our downstream consumers without changes - and if it
is org.apache.johnzon.core.JsonGeneratorFactoryImpl then it will create a
MetadataProviderOutputStream else a plain OutputStream and the feature will
be disabled (for now at least) - note that it can need a small indirection
like a MetadataProviderOutputStreamSupplier to not trigger the load of the
MetadataProvider on MapperBuilder class init/verification. This will also
use the snippet max size indeed. Here we want to ensure we can run without
johnzon-core as JSON-P impl.
4. Pass the supplier created at 3 to the Snippet class next to the
generator factory
5. Change JsonGeneratorFactoryImpl#createGenerator(OutputStream) to test if
it is a MetadataProvider instance to handle it (small trick there is to try
to cache the snippet buffer to keep reusing the buffer caching - and only
when needed):
public class JsonGeneratorFactoryImpl extends AbstractJsonFactory
implements JsonGeneratorFactory {
public static final String GENERATOR_BUFFER_LENGTH =
"org.apache.johnzon.default-char-buffer-generator";
public static final int DEFAULT_GENERATOR_BUFFER_LENGTH =
Integer.getInteger(GENERATOR_BUFFER_LENGTH, 64 * 1024); //64k
static final Collection<String> SUPPORTED_CONFIG_KEYS = asList(
JsonGenerator.PRETTY_PRINTING, GENERATOR_BUFFER_LENGTH,
BUFFER_STRATEGY
);
//key caching currently disabled
private final boolean pretty;
private final Buffer buffer;
private volatile Buffer firstCustomBuffer;
public JsonGeneratorFactoryImpl(final Map<String, ?> config) {
super(config, SUPPORTED_CONFIG_KEYS, null);
this.pretty = getBool(JsonGenerator.PRETTY_PRINTING, false);
final int bufferSize = getInt(GENERATOR_BUFFER_LENGTH,
DEFAULT_GENERATOR_BUFFER_LENGTH);
if (bufferSize <= 0) {
throw new IllegalArgumentException("buffer length must be
greater than zero");
}
this.buffer = new
Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
}
@Override
public JsonGenerator createGenerator(final Writer writer) {
return new JsonGeneratorImpl(writer, buffer.provider, pretty);
}
@Override
public JsonGenerator createGenerator(final OutputStream out) {
if (MetadataProvider.class.isInstance(out)) { // mainly for Snippet
handling since generators don't have properties
final int bufferSize =
MetadataProvider.class.cast(out).bufferSize();
if (buffer.size != bufferSize) {
// most of the time it will be a single aside buffer so
keep its factory to reuse the provider cache
if (firstCustomBuffer == null) {
synchronized (this) {
if (firstCustomBuffer == null) {
firstCustomBuffer = new
Buffer(getBufferProvider().newCharProvider(bufferSize), bufferSize);
}
}
}
Buffer customBuffer = firstCustomBuffer;
if (customBuffer.size != bufferSize) { // don't use the
cache, shouldn't occur often
return new JsonGeneratorImpl(out,
getBufferProvider().newCharProvider(bufferSize), pretty);
}
return new JsonGeneratorImpl(out, customBuffer.provider,
pretty);
} // else just reuse the original buffer since it matches and
leverages its cache
}
return new JsonGeneratorImpl(out, buffer.provider, pretty);
}
@Override
public JsonGenerator createGenerator(final OutputStream out, final
Charset charset) {
return new JsonGeneratorImpl(out,charset, buffer.provider, pretty);
}
@Override
public Map<String, ?> getConfigInUse() {
return Collections.unmodifiableMap(internalConfig);
}
private static final class Buffer {
private final BufferStrategy.BufferProvider<char[]> provider;
private final int size;
private Buffer(final BufferStrategy.BufferProvider<char[]>
bufferProvider, final int bufferSize) {
this.provider = bufferProvider;
this.size = bufferSize;
}
}
}
6. you use the outputstream supplier in Snippet and done
Wdyt?
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 26 avr. 2022 à 07:05, Romain Manni-Bucau <rm...@gmail.com> a
écrit :
>
>
> Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
> écrit :
>
>> Since we're now getting the snippet max length via the config, could we
>> construct the JsonGeneratorImpl directly and pass in a buffer of the
>> correct size?
>>
>
> From the JsonProvider we can - mapper must not depend on johnzon-core - at
> the condition to promote the other properties too AND not do it if user
> provided a custom generator factory which would just be reused in this
> case, so not sure it solves the issue.
>
>
>> -David
>>
>>
>> > On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <rm...@gmail.com>
>> wrote:
>> >
>> > No worries, got it.
>> >
>> > One issue copying the generator is that it will still miss the flush for
>> > all small objects and for all other cases the heuristic + a simple
>> > truncation if needed would work with almost no other downsides than not
>> > being exact but goal was to limit the overflow/cpu/mem so all good IMHO.
>> >
>> > Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com> a
>> > écrit :
>> >
>> >>> On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com>
>> >> wrote:
>> >>>
>> >>> The trick with that is there is yet another buffer being held
>> internally
>> >> by ObjectOutputStream and it recreates the issue.
>> >>
>> >> Just read this again -- this was supposed to be "OutputStreamWriter"
>> not
>> >> ObjectOutputStream. Too many years of EJB :)
>> >>
>> >>
>> >> -David
>> >>
>> >>
>>
>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le mar. 26 avr. 2022 à 00:44, David Blevins <da...@gmail.com> a
écrit :
> Since we're now getting the snippet max length via the config, could we
> construct the JsonGeneratorImpl directly and pass in a buffer of the
> correct size?
>
From the JsonProvider we can - mapper must not depend on johnzon-core - at
the condition to promote the other properties too AND not do it if user
provided a custom generator factory which would just be reused in this
case, so not sure it solves the issue.
> -David
>
>
> > On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > No worries, got it.
> >
> > One issue copying the generator is that it will still miss the flush for
> > all small objects and for all other cases the heuristic + a simple
> > truncation if needed would work with almost no other downsides than not
> > being exact but goal was to limit the overflow/cpu/mem so all good IMHO.
> >
> > Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com> a
> > écrit :
> >
> >>> On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com>
> >> wrote:
> >>>
> >>> The trick with that is there is yet another buffer being held
> internally
> >> by ObjectOutputStream and it recreates the issue.
> >>
> >> Just read this again -- this was supposed to be "OutputStreamWriter" not
> >> ObjectOutputStream. Too many years of EJB :)
> >>
> >>
> >> -David
> >>
> >>
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
Since we're now getting the snippet max length via the config, could we construct the JsonGeneratorImpl directly and pass in a buffer of the correct size?
-David
> On Apr 25, 2022, at 2:19 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> No worries, got it.
>
> One issue copying the generator is that it will still miss the flush for
> all small objects and for all other cases the heuristic + a simple
> truncation if needed would work with almost no other downsides than not
> being exact but goal was to limit the overflow/cpu/mem so all good IMHO.
>
> Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com> a
> écrit :
>
>>> On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com>
>> wrote:
>>>
>>> The trick with that is there is yet another buffer being held internally
>> by ObjectOutputStream and it recreates the issue.
>>
>> Just read this again -- this was supposed to be "OutputStreamWriter" not
>> ObjectOutputStream. Too many years of EJB :)
>>
>>
>> -David
>>
>>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
No worries, got it.
One issue copying the generator is that it will still miss the flush for
all small objects and for all other cases the heuristic + a simple
truncation if needed would work with almost no other downsides than not
being exact but goal was to limit the overflow/cpu/mem so all good IMHO.
Le lun. 25 avr. 2022 à 22:57, David Blevins <da...@gmail.com> a
écrit :
> > On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com>
> wrote:
> >
> > The trick with that is there is yet another buffer being held internally
> by ObjectOutputStream and it recreates the issue.
>
> Just read this again -- this was supposed to be "OutputStreamWriter" not
> ObjectOutputStream. Too many years of EJB :)
>
>
> -David
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
> On Apr 25, 2022, at 10:47 AM, David Blevins <da...@gmail.com> wrote:
>
> The trick with that is there is yet another buffer being held internally by ObjectOutputStream and it recreates the issue.
Just read this again -- this was supposed to be "OutputStreamWriter" not ObjectOutputStream. Too many years of EJB :)
-David
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
> On Apr 24, 2022, at 11:09 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Le dim. 24 avr. 2022 à 22:30, David Blevins <da...@gmail.com> a
> écrit :
>
>> All,
>>
>> I added more tests and found that most the optimizations were not
>> happening due to buffering.
>>
>> Essentially there are two buffers between Snippet.Buffer and
>> Snippet.SnippetOutputStream. The SnippetOutputStream had the
>> responsibility to tell the code up the stack when we've reached the max
>> snippet length. Since all the bytes were buffered, it would see nothing
>> until the very end and we'd end up serializing the full json text anyway.
>>
>> One is the 64k buffer in JsonGeneratorImpl and the other is an 8k buffer
>> in the JVM implementation code of OutputStreamWriter. Since the
>> OutputStreamWriter buffer is hardcoded, we can't solve this by adjusting
>> buffer sizes and have no choice but to aggressively call flush() to ensure
>> SnippetOutputStream has the bytes and can do its job.
>>
>
> Not sure I get that since if you close in a finally block the generator, it
> will flush the actual output and all will be good.
> But can be to call tostring to early rather than a buffering issue
It'd difficult to explain, but I'll do my best and thanks for the patience if my attempt is poor.
The code is designed with the assumption that as json is serialized there will be write calls made on SnippetOutputStream, which then counts the bytes and can eventually tell Snippet.Buffer to stop making more json via the 'snippet.terminate()' calls. In practice this doesn't happen.
In practice what does happen is the entire json document, up to a limit of 64k, will be created before any calls reach SnippetOutputStream. This is because JsonGeneratorImpl is holding a buffer (64k by default) and does not call any writes on the Writer instance it's holding until that buffer has filled or close is called.
My first instinct was to reduce that 64k buffer to the snippet max length and solve this problem that way. The trick with that is there is yet another buffer being held internally by ObjectOutputStream and it recreates the issue. That buffer is hardcoded to be 8k. So even if we adjust the JsonGeneratorImpl buffer size, in practice what happens is the entire json document, up to a limit of 8k, will be created before any calls reach SnippetOutputStream.
Certainly 8k is better than 64k which is better than potentially 1GB of json, but I wanted to try and get close to the spirit of what we were both after originally which is that we avoid serializing a lot of json only to throw most of it away and show just a chunk of it.
The only way to do that is the flush() calls. That's the only way to ensure SnippetOutputStream is getting the json data as we serialize it.
Hope some of this helps.
-David
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le dim. 24 avr. 2022 à 22:30, David Blevins <da...@gmail.com> a
écrit :
> All,
>
> I added more tests and found that most the optimizations were not
> happening due to buffering.
>
> Essentially there are two buffers between Snippet.Buffer and
> Snippet.SnippetOutputStream. The SnippetOutputStream had the
> responsibility to tell the code up the stack when we've reached the max
> snippet length. Since all the bytes were buffered, it would see nothing
> until the very end and we'd end up serializing the full json text anyway.
>
> One is the 64k buffer in JsonGeneratorImpl and the other is an 8k buffer
> in the JVM implementation code of OutputStreamWriter. Since the
> OutputStreamWriter buffer is hardcoded, we can't solve this by adjusting
> buffer sizes and have no choice but to aggressively call flush() to ensure
> SnippetOutputStream has the bytes and can do its job.
>
Not sure I get that since if you close in a finally block the generator, it
will flush the actual output and all will be good.
But can be to call tostring to early rather than a buffering issue ;)
> I've reworked the code and greatly expanded the tests so we're not just
> asserting the resulting text but the details in how we get there.
>
> We could potentially have Snippet override the buffer setting of
> JsonGeneratorFactoryImpl so the buffer is of snippetMaxLength. I'm curious
> as to what others think. On one hand, we could fit 1310 snippet buffers of
> 50 bytes into the size of just one 64k default buffer. On the other hand,
> we're coping config maps and essentially building a new factory to get
> there. I'm not sure if I like that idea.
>
Thing is that using a custom generator does not work in general until you
capture all generator factory properties which is equivalent to reuse the
same instance as explained earlier, this is why Snippet must not get any
owned/internal instance of generator/factory.
>
> > On Apr 21, 2022, at 10:39 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Le ven. 22 avr. 2022 à 01:00, David Blevins <da...@gmail.com> a
> > écrit :
> >> Where do we document these kinds of parameters? If you have any
> pointers
> >> I'll try to work that into the next PR along with using this in a few
> more
> >> places.
> >>
> >
> > We use maven site. so
> > https://github.com/apache/johnzon/blob/master/src/site/markdown/index.md
> > It is not great in terms of browsing but the place we have as of today.
> > Feel free to break it in several places if you think it is better.
>
> Great, thanks. Looks like we haven't yet started documenting the config
> properties that can be passed into JsonbConfig.
>
> I'll probably defer that then and move on to updating all the error
> messages in a second PR. Getting proper documentation for our JsonbConfig
> properties is likely to involve a larger effort to compile a list of all
> the properties and their defaults. That will definitely be work unto
> itself.
>
>
> -David
>
>
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le dim. 24 avr. 2022 à 22:30, David Blevins <da...@gmail.com> a
écrit :
> All,
>
> I added more tests and found that most the optimizations were not
> happening due to buffering.
>
> Essentially there are two buffers between Snippet.Buffer and
> Snippet.SnippetOutputStream. The SnippetOutputStream had the
> responsibility to tell the code up the stack when we've reached the max
> snippet length. Since all the bytes were buffered, it would see nothing
> until the very end and we'd end up serializing the full json text anyway.
>
> One is the 64k buffer in JsonGeneratorImpl and the other is an 8k buffer
> in the JVM implementation code of OutputStreamWriter. Since the
> OutputStreamWriter buffer is hardcoded, we can't solve this by adjusting
> buffer sizes and have no choice but to aggressively call flush() to ensure
> SnippetOutputStream has the bytes and can do its job.
>
> I've reworked the code and greatly expanded the tests so we're not just
> asserting the resulting text but the details in how we get there.
>
> We could potentially have Snippet override the buffer setting of
> JsonGeneratorFactoryImpl so the buffer is of snippetMaxLength. I'm curious
> as to what others think. On one hand, we could fit 1310 snippet buffers of
> 50 bytes into the size of just one 64k default buffer. On the other hand,
> we're coping config maps and essentially building a new factory to get
> there. I'm not sure if I like that idea.
>
>
> > On Apr 21, 2022, at 10:39 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Le ven. 22 avr. 2022 à 01:00, David Blevins <da...@gmail.com> a
> > écrit :
> >> Where do we document these kinds of parameters? If you have any
> pointers
> >> I'll try to work that into the next PR along with using this in a few
> more
> >> places.
> >>
> >
> > We use maven site. so
> > https://github.com/apache/johnzon/blob/master/src/site/markdown/index.md
> > It is not great in terms of browsing but the place we have as of today.
> > Feel free to break it in several places if you think it is better.
>
> Great, thanks. Looks like we haven't yet started documenting the config
> properties that can be passed into JsonbConfig.
>
Well we did at
https://github.com/apache/johnzon/blob/master/src/site/markdown/index.md#json-b-json-b-10-compliant
but this one is not key i guess
> I'll probably defer that then and move on to updating all the error
> messages in a second PR. Getting proper documentation for our JsonbConfig
> properties is likely to involve a larger effort to compile a list of all
> the properties and their defaults. That will definitely be work unto
> itself.
>
> -David
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
All,
I added more tests and found that most the optimizations were not happening due to buffering.
Essentially there are two buffers between Snippet.Buffer and Snippet.SnippetOutputStream. The SnippetOutputStream had the responsibility to tell the code up the stack when we've reached the max snippet length. Since all the bytes were buffered, it would see nothing until the very end and we'd end up serializing the full json text anyway.
One is the 64k buffer in JsonGeneratorImpl and the other is an 8k buffer in the JVM implementation code of OutputStreamWriter. Since the OutputStreamWriter buffer is hardcoded, we can't solve this by adjusting buffer sizes and have no choice but to aggressively call flush() to ensure SnippetOutputStream has the bytes and can do its job.
I've reworked the code and greatly expanded the tests so we're not just asserting the resulting text but the details in how we get there.
We could potentially have Snippet override the buffer setting of JsonGeneratorFactoryImpl so the buffer is of snippetMaxLength. I'm curious as to what others think. On one hand, we could fit 1310 snippet buffers of 50 bytes into the size of just one 64k default buffer. On the other hand, we're coping config maps and essentially building a new factory to get there. I'm not sure if I like that idea.
> On Apr 21, 2022, at 10:39 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Le ven. 22 avr. 2022 à 01:00, David Blevins <da...@gmail.com> a
> écrit :
>> Where do we document these kinds of parameters? If you have any pointers
>> I'll try to work that into the next PR along with using this in a few more
>> places.
>>
>
> We use maven site. so
> https://github.com/apache/johnzon/blob/master/src/site/markdown/index.md
> It is not great in terms of browsing but the place we have as of today.
> Feel free to break it in several places if you think it is better.
Great, thanks. Looks like we haven't yet started documenting the config properties that can be passed into JsonbConfig.
I'll probably defer that then and move on to updating all the error messages in a second PR. Getting proper documentation for our JsonbConfig properties is likely to involve a larger effort to compile a list of all the properties and their defaults. That will definitely be work unto itself.
-David
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le ven. 22 avr. 2022 à 01:00, David Blevins <da...@gmail.com> a
écrit :
> > On Apr 20, 2022, at 11:12 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Le jeu. 21 avr. 2022 à 03:04, David Blevins <db...@tomitribe.com> a
> > écrit :
> >
> >> - https://github.com/apache/johnzon/pull/84
> >
> > Commented a few details but it is overall *the* solution! Thanks David!
>
> Thanks so much for the collaboration on this. A simple thing turned out
> to be really cool!
>
> >> - The current default number of characters in a snippet is 50. We could
> >> potentially have a system property to allow users to get more.
> >>
> >
> > Nop, a JsonXxFactory property as all toggles ;)
> > +1 for it
>
> I went ahead and added that code you supplied -- thank you for that!
> Where do we document these kinds of parameters? If you have any pointers
> I'll try to work that into the next PR along with using this in a few more
> places.
>
We use maven site. so
https://github.com/apache/johnzon/blob/master/src/site/markdown/index.md
It is not great in terms of browsing but the place we have as of today.
Feel free to break it in several places if you think it is better.
>
> -David
>
>
Re: Showing a chunk of json in exceptions
Posted by David Blevins <da...@gmail.com>.
> On Apr 20, 2022, at 11:12 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Le jeu. 21 avr. 2022 à 03:04, David Blevins <db...@tomitribe.com> a
> écrit :
>
>> - https://github.com/apache/johnzon/pull/84
>
> Commented a few details but it is overall *the* solution! Thanks David!
Thanks so much for the collaboration on this. A simple thing turned out to be really cool!
>> - The current default number of characters in a snippet is 50. We could
>> potentially have a system property to allow users to get more.
>>
>
> Nop, a JsonXxFactory property as all toggles ;)
> +1 for it
I went ahead and added that code you supplied -- thank you for that! Where do we document these kinds of parameters? If you have any pointers I'll try to work that into the next PR along with using this in a few more places.
-David
Re: Showing a chunk of json in exceptions
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le jeu. 21 avr. 2022 à 03:04, David Blevins <db...@tomitribe.com> a
écrit :
> Hey All,
>
> With inspiration from Romain, I went ahead and wrote a Json serializer
> that can create short Json snippets that we can safely include in error
> messages. I.e. it is optimized to only write a specific number of bytes
> and then stop. The recursion is tightly controlled to stop early as well.
> You can see that code in this PR if you're curious:
>
> - https://github.com/apache/johnzon/pull/84
Commented a few details but it is overall *the* solution! Thanks David!
>
> A few thoughts on where we could go from here:
>
> - We can probably work this into more exception handling that currently
> doesn't print Json
>
+1
> - We do have some existing exception handling calling toString on
> JsonValue instances we should update. For example:
>
> https://github.com/apache/johnzon/blob/v1.2.16/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/MappingParserImpl.java#L627
Outch, assumption was that it was a primitive but you are right it is wrong
>
> - The current default number of characters in a snippet is 50. We could
> potentially have a system property to allow users to get more.
>
Nop, a JsonXxFactory property as all toggles ;)
+1 for it
>
> Thoughts?
>
>
> -David
>
>