Yes, some more logging or even better JMX would be a huge bonus! I thought about JMX as well. It could also be used to change the parameters at runtime even.
LieGrue, strub > Am 26.09.2017 um 10:25 schrieb Romain Manni-Bucau <[email protected]>: > > From what I looked (don't hesitate to shout if wrong, looked very quickly) > it works only because we release and go completely out of buffering > mecanism with strategies which support to bypass memory releasing. Said > otherwise it looks like you are right and it works good enough for a > release but we'll need to at least add some instrumentation in the > copyCurrentValue() method (where the comment says // not good at runtime > but handled) to expose through JMX or logs that we resize and give some > hint on how often it happens and what would be a good size. My worry about > that is to get very bad perf (don't know current state but when I added the > buffering we went 70% faster) and no way to see/control it from the end > user perspective. > > > Romain Manni-Bucau > @rmannibucau <https://twitter.com/rmannibucau> | Blog > <https://blog-rmannibucau.rhcloud.com> | Old Blog > <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > <https://javaeefactory-rmannibucau.rhcloud.com> > > 2017-09-26 10:16 GMT+02:00 Mark Struberg <[email protected]>: > >> I've reviewed the cache together with Reinhard in a hangout pair >> programming session on Friday and it looks good imo. >> >> LieGrue, >> strub >> >> >>> Am 26.09.2017 um 09:56 schrieb Romain Manni-Bucau <[email protected] >>> : >>> >>> then the only blocker is to guarantee we will never add back to any cache >>> the extended buffer. This can just be a matter of reviewing our >> strategies >>> but didn't get a chance to do it yet. Can surely have a look this >> week-end >>> but fear i'll not be able before :( >>> >>> >>> Romain Manni-Bucau >>> @rmannibucau <https://twitter.com/rmannibucau> | Blog >>> <https://blog-rmannibucau.rhcloud.com> | Old Blog >>> <http://rmannibucau.wordpress.com> | Github <https://github.com/ >> rmannibucau> | >>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory >>> <https://javaeefactory-rmannibucau.rhcloud.com> >>> >>> 2017-09-26 9:22 GMT+02:00 Mark Struberg <[email protected]>: >>> >>>> Hmm maybe, but in a later version, ok? >>>> >>>> Would love to start the release today, ok? >>>> >>>> LieGrue, >>>> Strub >>>> >>>>> Am 26.09.2017 um 08:10 schrieb Romain Manni-Bucau < >> [email protected] >>>>> : >>>>> >>>>> We already have it, what is important with this auto extend logic is a >>>>> toggle flag to add back in the cache either the original ("small") >> buffer >>>>> or the extended one ("big"). We dont have that support yet. But in all >>>>> cases we should call release to let the strategy have a chance to >> cleanup >>>>> things. >>>>> >>>>> Le 26 sept. 2017 08:01, "Mark Struberg" <[email protected]> a >>>>> écrit : >>>>> >>>>>> Hmm, what about a keepMaxBuffers = n config? >>>>>> >>>>>> By default it would be e.g. 50. >>>>>> Means if the queue is > n we do not return it. >>>>>> By configure 0 as n we automatically have what you want, right? >>>>>> >>>>>> LieGrue, >>>>>> strub >>>>>> >>>>>> >>>>>>> Am 26.09.2017 um 06:59 schrieb Romain Manni-Bucau < >>>> [email protected] >>>>>>> : >>>>>>> >>>>>>> We should get a config about it and ensure we release() *any* buffer >> in >>>>>>> case we go native (validate our lifecycle). This mean we should >>>>>> distinguish >>>>>>> between release (end of buffer usage) and going back to pool (or the >>>>>>> opposite: newPoolableBuffer vs newVolatileBuffer). >>>>>>> >>>>>>> The test is not super good yet but a start, it misses some size >>>> asserts. >>>>>>> >>>>>>> >>>>>>> Le 25 sept. 2017 23:44, "Mark Struberg" <[email protected]> >> a >>>>>>> écrit : >>>>>>> >>>>>>>> Not quite sure what this should tell us. >>>>>>>> >>>>>>>> Of course the exceeded buffer doesn't get returned! >>>>>>>> We might probably return the original one and thus improve our >> logic. >>>>>>>> But it's not strictly wrong the way it currently behaves. >>>>>>>> It's now way faster under normal conditions and it consumes way less >>>> mem >>>>>>>> than before. >>>>>>>> >>>>>>>> LieGrue, >>>>>>>> strub >>>>>>>> >>>>>>>>> Am 25.09.2017 um 08:22 schrieb [email protected]: >>>>>>>>> >>>>>>>>> Repository: johnzon >>>>>>>>> Updated Branches: >>>>>>>>> refs/heads/master cc24e8e1c -> 60f48cf65 >>>>>>>>> >>>>>>>>> >>>>>>>>> adding a broken test to show why previous commit broke the buffer >>>>>>>> strategies >>>>>>>>> >>>>>>>>> >>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/johnzon/repo >>>>>>>>> Commit: http://git-wip-us.apache.org/repos/asf/johnzon/commit/ >>>> 60f48cf6 >>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/johnzon/tree/60f48cf6 >>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/johnzon/diff/60f48cf6 >>>>>>>>> >>>>>>>>> Branch: refs/heads/master >>>>>>>>> Commit: 60f48cf65708e3a9bc9943b6d5cac0435772b6e9 >>>>>>>>> Parents: cc24e8e >>>>>>>>> Author: Romain Manni-Bucau <[email protected]> >>>>>>>>> Authored: Mon Sep 25 08:22:26 2017 +0200 >>>>>>>>> Committer: Romain Manni-Bucau <[email protected]> >>>>>>>>> Committed: Mon Sep 25 08:22:26 2017 +0200 >>>>>>>>> >>>>>>>>> ------------------------------------------------------------ >>>> ---------- >>>>>>>>> .../apache/johnzon/core/BrokenDefaultTest.java | 101 >>>>>>>> +++++++++++++++++++ >>>>>>>>> 1 file changed, 101 insertions(+) >>>>>>>>> ------------------------------------------------------------ >>>> ---------- >>>>>>>>> >>>>>>>>> >>>>>>>>> http://git-wip-us.apache.org/repos/asf/johnzon/blob/ >>>>>>>> 60f48cf6/johnzon-core/src/test/java/org/apache/johnzon/ >>>>>>>> core/BrokenDefaultTest.java >>>>>>>>> ------------------------------------------------------------ >>>> ---------- >>>>>>>>> diff --git a/johnzon-core/src/test/java/org/apache/johnzon/core/ >>>>>> BrokenDefaultTest.java >>>>>>>> b/johnzon-core/src/test/java/org/apache/johnzon/core/ >>>>>>>> BrokenDefaultTest.java >>>>>>>>> new file mode 100644 >>>>>>>>> index 0000000..ec63d9f >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/johnzon-core/src/test/java/org/apache/johnzon/core/ >>>>>>>> BrokenDefaultTest.java >>>>>>>>> @@ -0,0 +1,101 @@ >>>>>>>>> +/* >>>>>>>>> + * Licensed to the Apache Software Foundation (ASF) under one or >>>> more >>>>>>>>> + * contributor license agreements. See the NOTICE file distributed >>>>>> with >>>>>>>>> + * this work for additional information regarding copyright >>>> ownership. >>>>>>>>> + * The ASF licenses this file to You under the Apache License, >>>> Version >>>>>>>> 2.0 >>>>>>>>> + * (the "License"); you may not use this file except in compliance >>>>>> with >>>>>>>>> + * the License. You may obtain a copy of the License at >>>>>>>>> + * >>>>>>>>> + * http://www.apache.org/licenses/LICENSE-2.0 >>>>>>>>> + * >>>>>>>>> + * Unless required by applicable law or agreed to in writing, >>>> software >>>>>>>>> + * distributed under the License is distributed on an "AS IS" >> BASIS, >>>>>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>>>>>>> implied. >>>>>>>>> + * See the License for the specific language governing permissions >>>> and >>>>>>>>> + * limitations under the License. >>>>>>>>> + */ >>>>>>>>> +package org.apache.johnzon.core; >>>>>>>>> + >>>>>>>>> +import static java.util.Collections.emptyMap; >>>>>>>>> +import static org.junit.Assert.assertEquals; >>>>>>>>> + >>>>>>>>> +import java.io.ByteArrayInputStream; >>>>>>>>> +import java.io.IOException; >>>>>>>>> +import java.io.InputStream; >>>>>>>>> +import java.lang.reflect.Field; >>>>>>>>> +import java.nio.charset.StandardCharsets; >>>>>>>>> +import java.util.Queue; >>>>>>>>> + >>>>>>>>> +import javax.json.Json; >>>>>>>>> +import javax.json.stream.JsonParser; >>>>>>>>> +import javax.json.stream.JsonParserFactory; >>>>>>>>> + >>>>>>>>> +import org.junit.Ignore; >>>>>>>>> +import org.junit.Test; >>>>>>>>> + >>>>>>>>> +public class BrokenDefaultTest { >>>>>>>>> + >>>>>>>>> + @Test >>>>>>>>> + @Ignore("buggy but pushing to share the use case") >>>>>>>>> + public void run() throws NoSuchFieldException, >>>>>>>> IllegalAccessException { // shouldnt fail by default >>>>>>>>> + final JsonParserFactory factory = >> Json.createParserFactory( >>>>>>>> emptyMap()); >>>>>>>>> + final int length = 1024 * 1024; >>>>>>>>> + assertEquals(0, get(Queue.class, get( >>>>>>>>> + BufferStrategy.BufferProvider.class, factory, >>>>>>>> "bufferProvider"), "queue").size()); >>>>>>>>> + try (final JsonParser parser = factory.createParser( >>>>>> newDynamicInput(length))) >>>>>>>> { >>>>>>>>> + int eventCount = 0; >>>>>>>>> + while (parser.hasNext()) { >>>>>>>>> + eventCount++; >>>>>>>>> + final JsonParser.Event next = parser.next(); >>>>>>>>> + if (eventCount == 2 && next == >>>>>>>> JsonParser.Event.VALUE_STRING) { >>>>>>>>> + assertEquals(length, >>>> parser.getString().length()); >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + assertEquals(1, get(Queue.class, get( >>>>>>>>> + BufferStrategy.BufferProvider.class, factory, >>>>>>>> "bufferProvider"), "queue").size()); >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + private <T> T get(final Class<T> returnType, final Object >>>>>> instance, >>>>>>>> final String field) >>>>>>>>> + throws NoSuchFieldException, IllegalAccessException { >>>>>>>>> + Class<?> current = instance.getClass(); >>>>>>>>> + while (current != Object.class) { >>>>>>>>> + try { >>>>>>>>> + final Field declaredField = >>>> current.getDeclaredField( >>>>>>>> field); >>>>>>>>> + if (!declaredField.isAccessible()) { >>>>>>>>> + declaredField.setAccessible(true); >>>>>>>>> + } >>>>>>>>> + return returnType.cast(declaredField. >>>> get(instance)); >>>>>>>>> + } catch (final NoSuchFieldException nsfe) { >>>>>>>>> + current = current.getSuperclass(); >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + throw new IllegalAccessError(instance + " field: " + >> field); >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + private InputStream newDynamicInput(final int size) { >>>>>>>>> + return new InputStream() { >>>>>>>>> + >>>>>>>>> + private InputStream before = new >>>>>>>> ByteArrayInputStream("{\"key\":\"".getBytes( >> StandardCharsets.UTF_8)); >>>>>>>>> + >>>>>>>>> + private InputStream after = new >>>>>> ByteArrayInputStream("\"}". >>>>>>>> getBytes(StandardCharsets.UTF_8)); >>>>>>>>> + >>>>>>>>> + private int remaining = size; >>>>>>>>> + >>>>>>>>> + @Override >>>>>>>>> + public int read() throws IOException { >>>>>>>>> + { >>>>>>>>> + final int val = before.read(); >>>>>>>>> + if (val >= 0) { >>>>>>>>> + return val; >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + if (remaining < 0) { >>>>>>>>> + return after.read(); >>>>>>>>> + } >>>>>>>>> + remaining--; >>>>>>>>> + return 'a'; >>>>>>>>> + } >>>>>>>>> + }; >>>>>>>>> + } >>>>>>>>> +} >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> . >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >>
