@Fikavec
Can you please post the above comments on the PR? It would serve as a record
On Thu, Mar 9, 2023 at 9:07 AM Fikavec F <fika...@yandex.ru> wrote:
> Thank you for your work Noble Paul, it's very interesting.
> You were so attentive that you noticed and replaced "else if (val
> instanceof Double) { gen.writeNumber(val.floatValue()); }" with "else if
> (val instanceof Double) { gen.writeNumber(val.doubleValue()); }". But I
> just copied the code (Line 70 -
> https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/response/SmileResponseWriter.java#L70)
> and
> fasterxml.jackson SmileGenerator supports double (in code line 1352 -
> https://github.com/FasterXML/jackson-dataformat-smile/blob/master/src/main/java/com/fasterxml/jackson/dataformat/smile/SmileGenerator.java#L1352
> and
> in specification
> https://github.com/FasterXML/smile-format-specification/blob/master/smile-specification.md#token-class-simple-literals-numbers)
> It
> turns out that you found and fixed a bug in Solr SmileResponseWriter.java -
> I'm applaud, will you fix the SmileResponseWriter or report it?
> I noticed that in every JacksonJsonWriter write function call there is
> a call "WriterImpl sw = new WriterImpl(" and as a consequence, the creation
> of "JsonFactory JsonFactory = new JsonFactory();" (line 56 -
> https://github.com/apache/solr/blob/231475637e07d057d2ae1bfbe9e41e5b85b1cba5/solr/core/src/java/org/apache/solr/response/JacksonJsonWriter.java#L56)
> but
> in the jackson documentation I read the following [Factory instances are
> thread-safe and reusable after configuration (if any). ... Factory reuse is
> important if efficiency matters; most recycling of expensive construct is
> done on per-factory basis.] (
> https://fasterxml.github.io/jackson-core/javadoc/2.13/com/fasterxml/jackson/core/JsonFactory.html)
> As
> far as I understand, what "write" is called quite often in a query-loaded
> server, we can't avoid permanent JsonFactory creations as recommended by
> the library authors? Maybe move it to class constructor or are there
> reasons why this is bad for Solr?
> Is writeStrRaw just to check whether to use (gen.writeRawValue(val) or
> gen.writeRaw(val)) in writeStr? (I'm just already using my implementation
> and if necessary, I'll fix it in it.)
>
> The wrk benchmarking tool tests also showed some improvements. From 10%
> (with gzip) to 30% (without gzip) increase in rps when searching for random
> words in a big text field and retrieve documents. For a lot of very small
> queries with gzip enabled, the difference is not very noticeable, since the
> json output is a small percentage of the query execution time (search, data
> collection from disk and shards, gzip). But the larger the size of the
> responses received (most of all when there are few but large fields in the
> response), the more noticeable the difference, especially when the cursor
> passes through a large collection with the full data fetching. Today it
> turned out that increasing outputBufferSize and outputAggregationSize in
> jetty.xml additionally affects the speed and, it seems, allows you to
> increase it even more, I will take measurements in the near future and
> present the results.
>
> Best Regards,
>
>
--
-----------------------------------------------------
Noble Paul
