[client] reduce the number of allocations #105

truthbk · 2020-03-09T23:06:09Z

This is an adaptation of #74 by @njhill by which we take the same principles and apply them to the new client architecture to reduce the overhead of allocations on the client, and consequently on the client application.

Still WIP - a couple of things went wrong after a rebase, so a few things still pending fixes.

njhill · 2020-03-11T01:12:46Z

@truthbk I started to review yesterday before you made further changes and had actually already written a comment suggesting to have a separate class for the sender Runnables as I see you've now added! Curious why you added separate lists for the StringBuilders/CharBuffers though - why not just have a dedicated instance in each class (in instance fields)? Then you could move the writeBuilderToSendBuffer method to that class too, which would allow for better reuse of the CharBuffers as in the original PR.

I'd also suggest to move other things into this class so that each worker has its own instance since they are not threadsafe:

The CharsetEncoder (utf8Encoder)
The NumberFormat instances

Anyhow let me know when it is ready and I'll take a look again :)

truthbk · 2020-03-11T14:06:25Z

Thank you for taking a look @njhill! The goal is to do all of that too! I'm fixing things one step at a time. Don't spend too much time reviewing this until the WIP tag is removed from the PR :)

In any case, thank you for the feedback, I had the exact same thoughts in mind to avoid code duplication and ensure a clean code structure. I still hadn't gone into verifying the thready safety of CharsetEncoder and NumberFormat, so thank you for that.

I'll let you know when this is ready!

ogaca-dd

LGTM: I did not spot anything wrong but it is easy to miss something. You may want to check if this code is well tested.

ogaca-dd · 2020-03-16T10:00:11Z

src/main/java/com/timgroup/statsd/NonBlockingStatsDClient.java

-            final NumberFormat numberFormatter = NumberFormat.getInstance(Locale.US);
-            numberFormatter.setGroupingUsed(false);
-            numberFormatter.setMaximumFractionDigits(6);
+    static {


Is it needed?

ogaca-dd · 2020-03-16T10:32:01Z

src/test/java/com/timgroup/statsd/NonBlockingStatsDClientTest.java

@@ -753,6 +756,7 @@ public void sends_too_large_message() throws Exception {
            final Exception exception = exceptions.get(0);
            assertEquals(InvalidMessageException.class, exception.getClass());
            assertTrue(((InvalidMessageException)exception).getInvalidMessage().startsWith("_sc|toolong|"));
+            // assertEquals(BufferOverflowException.class, exception.getClass());


truthbk · 2020-03-19T02:46:38Z

@njhill open for review if you want to chime in! :)

njhill

@truthbk looks good mostly but left a few comments

njhill · 2020-03-19T03:07:15Z

src/main/java/com/timgroup/statsd/Message.java

+interface Message {
+    /**
+     * Write this message to the provided {@link StringBuilder}. Will
+     * only ever be called from the sender thread.


Would be more accurate to say "a" sender thread now

njhill · 2020-03-19T03:08:13Z

src/main/java/com/timgroup/statsd/NonBlockingStatsDClient.java

-            final NumberFormat numberFormatter = NumberFormat.getInstance(Locale.US);
-            numberFormatter.setGroupingUsed(false);
-            numberFormatter.setMaximumFractionDigits(6);
+    static {


njhill · 2020-03-19T03:27:49Z

src/main/java/com/timgroup/statsd/NonBlockingStatsDClient.java

     */
-    private static final ThreadLocal<NumberFormat> NUMBER_FORMATTERS = new ThreadLocal<NumberFormat>() {
+    private static final ThreadLocal<NumberFormat> NUMBER_FORMATTER = new ThreadLocal<NumberFormat>() {


Why not also have the formatters as fields in ProcessingTask instead of ThreadLocals?

njhill · 2020-03-19T03:29:20Z

src/main/java/com/timgroup/statsd/StatsDProcessor.java

    protected static final String MESSAGE_TOO_LONG = "Message longer than size of sendBuffer";
    protected static final int WAIT_SLEEP_MS = 10;  // 10 ms would be a 100HZ slice

    protected final StatsDClientErrorHandler handler;

    protected final BufferPool bufferPool;
+    protected final List<StringBuilder> builders; // StringBuilders for processing, 1 per worker
+    protected final List<CharBuffer> charBuffers; // CharBuffers for processing, 1 per worker


I think these two are now obsolete?

njhill · 2020-03-19T03:29:40Z

src/main/java/com/timgroup/statsd/StatsDProcessor.java

+            CharBuffer buffer = CharBuffer.wrap(builder);
+            builders.add(builder);
+            charBuffers.add(buffer);
+        }


Now obsolete?

njhill · 2020-03-19T03:43:14Z

src/main/java/com/timgroup/statsd/StatsDNonBlockingProcessor.java

    private final int qcapacity;
    private final AtomicInteger qsize;  // qSize will not reflect actual size, but a close estimate.

+    private class ProcessingTask extends StatsDProcessor.ProcessingTask {


What's the difference between the two impls, they look pretty similar... could the common logic be de-duped i.e. moved into the superclass?

They're pretty close but the logic dealing with the queues blocking vs non-blocking is a little different. I might clean that up, but would rather do it in a separate PR.

njhill · 2020-03-19T03:44:53Z

src/main/java/com/timgroup/statsd/StatsDBlockingProcessor.java

+
+                    if (Thread.interrupted()) {
+                        return;
+                    }


This if block not needed

cleared it out.

njhill · 2020-03-19T03:46:28Z

src/main/java/com/timgroup/statsd/StatsDBlockingProcessor.java

+                return;
+            }
+
+            while (!(messages.isEmpty() && shutdown)) {


This is probably better as !(shutdown && messages.isEmpty()) to remove unnecessary queue contention

njhill · 2020-03-19T03:57:20Z

src/main/java/com/timgroup/statsd/StatsDBlockingProcessor.java

+                        try {
+                            writeBuilderToSendBuffer(sendBuffer);
+                        } catch (BufferOverflowException boe) {
+                            outboundQueue.put(sendBuffer);


I think sendBuffer.reset() is needed here as I had in the original PR, otherwise the message will span two packets (same below)

Ummm, not sure. that breaks the tests too, I'll dig a little.

It has no effect, tried a bunch of tests same packet count, same message count. Resetting the bottom one will actually break things. So I believe we don't need the resets at all.

njhill · 2020-03-19T04:04:44Z

src/main/java/com/timgroup/statsd/NonBlockingStatsDClient.java

-     * http://stackoverflow.com/a/1285297/2648
-     * https://github.com/indeedeng/java-dogstatsd-client/issues/4
+     * The NumberFormat instances are not threadsafe but are only ever called from
+     * the sender thread.


This comment now obsolete

Updated it.

truthbk · 2020-03-20T14:47:29Z

Gotta fix a few things after the rebase to telemetry - don't panic! 😉

This is an optimization to reduce the amount of objects allocated when the various metrics are recorded, and minimize processing done on the application threads. The various events are now passed in "raw" form via dedicated objects to the sender thread (rather than first being serialized to a string on the app thread). All of the serialization work is now done on the sender thread, avoiding allocation of any new strings or byte arrays. A single StringBuilder is reused in conjunction with the existing ByteBuffer. The NumberFormat ThreadLocals are no longer needed since they will now only be used by the one thread. Also included are a couple of thread-safety tweaks to the unit tests.

truthbk force-pushed the jaime/less_alloc branch from 613bbe4 to 6cf2806 Compare March 10, 2020 04:00

truthbk added the wip label Mar 10, 2020

truthbk force-pushed the jaime/less_alloc branch from c99049b to 1be0566 Compare March 11, 2020 00:49

truthbk mentioned this pull request Mar 11, 2020

Reduce overhead of metrics recording #74

Closed

truthbk removed the wip label Mar 12, 2020

truthbk mentioned this pull request Mar 12, 2020

Adding dogstatsd telemetry to the client #97

Merged

ogaca-dd approved these changes Mar 17, 2020

View reviewed changes

njhill reviewed Mar 19, 2020

View reviewed changes

truthbk force-pushed the jaime/less_alloc branch from d5cd46b to 6f859d9 Compare March 19, 2020 22:41

hush-hush approved these changes Mar 20, 2020

View reviewed changes

njhill and others added 9 commits April 8, 2020 22:01

cleanup, refactor, bad rebase fixes

cd1979b

StatsdProcessor abstract with all the goodies + thread safety

72463f0

Do not even attempt long messages + checkstyle

efd15dc

Message: adding missing class

d3efe64

NumberFormatter: use ThreadLocal for thread safety

d160c1f

[telemetry] fix to match new messaging

c75fd39

addressing feedback and cleanup

7024d1a

[tests] adding packet counts to dummy server

043615b

truthbk force-pushed the jaime/less_alloc branch from b1e7aa4 to 043615b Compare April 9, 2020 02:16

fix rebase collateral

269f01e

truthbk merged commit 28055c0 into master Apr 9, 2020

truthbk deleted the jaime/less_alloc branch April 9, 2020 02:52

truthbk added this to the 2.10.0 milestone May 5, 2020

truthbk mentioned this pull request May 5, 2020

Reduce heap and GC overhead #84

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[client] reduce the number of allocations #105

[client] reduce the number of allocations #105

truthbk commented Mar 9, 2020 •

edited

Loading

njhill commented Mar 11, 2020

truthbk commented Mar 11, 2020

ogaca-dd left a comment

ogaca-dd Mar 16, 2020

njhill Mar 19, 2020

ogaca-dd Mar 16, 2020

truthbk commented Mar 19, 2020

njhill left a comment

njhill Mar 19, 2020

njhill Mar 19, 2020

njhill Mar 19, 2020

njhill Mar 19, 2020

truthbk Mar 20, 2020

njhill Mar 19, 2020

truthbk Mar 20, 2020

njhill Mar 19, 2020

truthbk Mar 20, 2020

njhill Mar 19, 2020

truthbk Mar 20, 2020

njhill Mar 19, 2020

njhill Mar 19, 2020

truthbk Mar 20, 2020 •

edited

Loading

truthbk Mar 20, 2020

njhill Mar 19, 2020

truthbk Mar 20, 2020

truthbk commented Mar 20, 2020

[client] reduce the number of allocations #105

[client] reduce the number of allocations #105

Conversation

truthbk commented Mar 9, 2020 • edited Loading

njhill commented Mar 11, 2020

truthbk commented Mar 11, 2020

ogaca-dd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

truthbk commented Mar 19, 2020

njhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

truthbk Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

truthbk commented Mar 20, 2020

truthbk commented Mar 9, 2020 •

edited

Loading

truthbk Mar 20, 2020 •

edited

Loading