Packet improvement

Dynamic and precise rdata buffer allocation.

Have you benchmarked the difference in memory and in performance? Dynamic buffers can save a few bytes of memory, but it might have performance impact (since you have to pre-calculate the size and allocate the memory somewhere - does mm_alloc take chunks from heap? it could also lead to a bigger memory fragmentation)

I am aware of the pros and cons, but this was requested by Jan Vcelak since he thought allocating up to ~64KiB on the stack wasn't safe.

Alrighty - I still think we should benchmark the impacts before making such changes... @thlavacek

Done. But it is hard to decide whether there is a significant impact or not. Look at this chart: http://aule.elfove.cz/~brill/malloc.png (6 measurements, 3 with the commit before - green, 3 after commit - red, x=req/s sent, y=ans/s). If you ask me I would say that it is almost the same (according to two-sample t-test applied on max values from all green and red measurements; in fact p-val was 0.65, which means that from this measurements we have absolutely no evidence against the hypothesis that the stack->malloc didn't have any impact). But if you want to be sure, I can measure more points and use more convincing statistics (Holm-Bonfferoni method, perhaps?).

Raw data:

before 0eb3a8ee2aae63fd37fe855d44849365cab41135

 16:33#   | knotd 99.99% answered for 99518 (100000) q/s (=99508.04 a/s, 134.31B avg.resplen, 15 reps)
 16:33#   | knotd 99.99% answered for 198412 (200000) q/s (=198392.15 a/s, 134.31B avg.resplen, 30 reps)
 16:34#   | knotd 99.99% answered for 296784 (300000) q/s (=296754.32 a/s, 134.31B avg.resplen, 45 reps)
 16:34#   | knotd 93.51% answered for 393056 (400000) q/s (=367546.66 a/s, 134.31B avg.resplen, 60 reps)
 16:35#   | knotd 73.06% answered for 479080 (500000) q/s (=350015.84 a/s, 134.33B avg.resplen, 75 reps)
 16:36#   | knotd 55.17% answered for 584131 (600000) q/s (=322265.07 a/s, 134.34B avg.resplen, 90 reps)
 16:36#   | knotd 46.68% answered for 678294 (700000) q/s (=316627.63 a/s, 134.32B avg.resplen, 105 reps)
 16:37#   | knotd 38.14% answered for 754598 (800000) q/s (=287803.67 a/s, 134.30B avg.resplen, 120 reps)
 16:37#   | knotd 35.98% answered for 840336 (900000) q/s (=302352.89 a/s, 134.31B avg.resplen, 135 reps)
 16:38#   | knotd 31.21% answered for 894721 (1000000) q/s (=279242.42 a/s, 134.30B avg.resplen, 150 reps)

 19:56#   | knotd 99.99% answered for 99518 (100000) q/s (=99508.04 a/s, 134.31B avg.resplen, 15 reps)
 19:57#   | knotd 99.99% answered for 198412 (200000) q/s (=198392.15 a/s, 134.31B avg.resplen, 30 reps)
 19:57#   | knotd 99.99% answered for 296833 (300000) q/s (=296803.31 a/s, 134.31B avg.resplen, 45 reps)
 19:58#   | knotd 99.99% answered for 393120 (400000) q/s (=393080.68 a/s, 134.31B avg.resplen, 60 reps)
 19:58#   | knotd 80.17% answered for 479616 (500000) q/s (=384508.14 a/s, 134.32B avg.resplen, 75 reps)
 19:59#   | knotd 60.68% answered for 591910 (600000) q/s (=359170.98 a/s, 134.32B avg.resplen, 90 reps)
 20:00#   | knotd 50.76% answered for 678075 (700000) q/s (=344190.87 a/s, 134.30B avg.resplen, 105 reps)
 20:00#   | knotd 41.22% answered for 767386 (800000) q/s (=316316.50 a/s, 134.32B avg.resplen, 120 reps)
 20:01#   | knotd 38.40% answered for 831152 (900000) q/s (=319162.36 a/s, 134.30B avg.resplen, 135 reps)
 20:01#   | knotd 34.08% answered for 894587 (1000000) q/s (=304875.24 a/s, 134.29B avg.resplen, 150 reps)

 21:05#   | knotd 99.99% answered for 99518 (100000) q/s (=99508.04 a/s, 134.31B avg.resplen, 15 reps)
 21:06#   | knotd 99.99% answered for 198445 (200000) q/s (=198425.15 a/s, 134.31B avg.resplen, 30 reps)
 21:06#   | knotd 99.99% answered for 296784 (300000) q/s (=296754.32 a/s, 134.31B avg.resplen, 45 reps)
 21:07#   | knotd 99.99% answered for 392991 (400000) q/s (=392951.70 a/s, 134.31B avg.resplen, 60 reps)
 21:08#   | knotd 88.56% answered for 478850 (500000) q/s (=424069.56 a/s, 134.32B avg.resplen, 75 reps)
 21:08#   | knotd 65.53% answered for 583941 (600000) q/s (=382656.53 a/s, 134.31B avg.resplen, 90 reps)
 21:09#   | knotd 55.08% answered for 668470 (700000) q/s (=368193.27 a/s, 134.31B avg.resplen, 105 reps)
 21:09#   | knotd 45.04% answered for 756262 (800000) q/s (=340620.40 a/s, 134.30B avg.resplen, 120 reps)
 21:10#   | knotd 41.41% answered for 831536 (900000) q/s (=344339.05 a/s, 134.32B avg.resplen, 135 reps)
 21:10#   | knotd 37.06% answered for 893921 (1000000) q/s (=331287.12 a/s, 134.30B avg.resplen, 150 reps)


after 829ccc24615c600eea2575fe9bdfe6007d7d6b77


 16:17#   | knotd 99.99% answered for 99518 (100000) q/s (=99508.04 a/s, 134.31B avg.resplen, 15 reps)
 16:18#   | knotd 99.99% answered for 198445 (200000) q/s (=198425.15 a/s, 134.31B avg.resplen, 30 reps)
 16:18#   | knotd 99.99% answered for 296784 (300000) q/s (=296754.32 a/s, 134.31B avg.resplen, 45 reps)
 16:19#   | knotd 99.99% answered for 392991 (400000) q/s (=392951.70 a/s, 134.31B avg.resplen, 60 reps)
 16:19#   | knotd 81.08% answered for 479233 (500000) q/s (=388562.11 a/s, 134.33B avg.resplen, 75 reps)
 16:20#   | knotd 60.50% answered for 583941 (600000) q/s (=353284.30 a/s, 134.32B avg.resplen, 90 reps)
 16:21#   | knotd 50.39% answered for 669322 (700000) q/s (=337271.35 a/s, 134.30B avg.resplen, 105 reps)
 16:21#   | knotd 41.14% answered for 754005 (800000) q/s (=310197.65 a/s, 134.30B avg.resplen, 120 reps)
 16:22#   | knotd 38.34% answered for 827713 (900000) q/s (=317345.16 a/s, 134.31B avg.resplen, 135 reps)
 16:22#   | knotd 33.42% answered for 895522 (1000000) q/s (=299283.45 a/s, 134.30B avg.resplen, 150 reps)

 19:32#   | knotd 99.99% answered for 99518 (100000) q/s (=99508.04 a/s, 134.31B avg.resplen, 15 reps)
 19:33#   | knotd 99.99% answered for 198412 (200000) q/s (=198392.15 a/s, 134.31B avg.resplen, 30 reps)
 19:33#   | knotd 99.99% answered for 296882 (300000) q/s (=296852.31 a/s, 134.31B avg.resplen, 45 reps)
 19:34#   | knotd 92.81% answered for 393056 (400000) q/s (=364795.27 a/s, 134.31B avg.resplen, 60 reps)
 19:34#   | knotd 72.75% answered for 478621 (500000) q/s (=348196.77 a/s, 134.32B avg.resplen, 75 reps)
 19:35#   | knotd 54.56% answered for 584510 (600000) q/s (=318908.65 a/s, 134.32B avg.resplen, 90 reps)
 19:35#   | knotd 46.39% answered for 678185 (700000) q/s (=314610.02 a/s, 134.31B avg.resplen, 105 reps)
 19:36#   | knotd 37.79% answered for 766528 (800000) q/s (=289670.93 a/s, 134.30B avg.resplen, 120 reps)
 19:37#   | knotd 35.67% answered for 827586 (900000) q/s (=295199.92 a/s, 134.30B avg.resplen, 135 reps)
 19:37#   | knotd 30.78% answered for 897934 (1000000) q/s (=276384.08 a/s, 134.31B avg.resplen, 150 reps)

 20:15#   | knotd 99.99% answered for 99518 (100000) q/s (=99508.04 a/s, 134.31B avg.resplen, 15 reps)
 20:16#   | knotd 99.99% answered for 198445 (200000) q/s (=198425.15 a/s, 134.31B avg.resplen, 30 reps)
 20:16#   | knotd 99.99% answered for 296638 (300000) q/s (=296608.33 a/s, 134.31B avg.resplen, 45 reps)
 20:17#   | knotd 99.99% answered for 393120 (400000) q/s (=393080.68 a/s, 134.31B avg.resplen, 60 reps)
 20:17#   | knotd 82.86% answered for 479386 (500000) q/s (=397219.23 a/s, 134.32B avg.resplen, 75 reps)
 20:18#   | knotd 62.59% answered for 582901 (600000) q/s (=364837.73 a/s, 134.33B avg.resplen, 90 reps)
 20:19#   | knotd 51.66% answered for 671355 (700000) q/s (=346821.99 a/s, 134.31B avg.resplen, 105 reps)
 20:19#   | knotd 42.65% answered for 753532 (800000) q/s (=321381.39 a/s, 134.30B avg.resplen, 120 reps)
 20:20#   | knotd 39.19% answered for 843091 (900000) q/s (=330407.36 a/s, 134.30B avg.resplen, 135 reps)
 20:20#   | knotd 34.92% answered for 893788 (1000000) q/s (=312110.76 a/s, 134.30B avg.resplen, 150 reps)

I was kinda hoping you would read the rest of the comments here and not do the benchmark on this commit. The following commits should have a lot more impact on performance. Please try it again with the latest commit in this branch. Thanks.

Done. Now I compared head of packet_improvement branch against v1.99.1 tag. I believe that simplest way to do it is Hotelling's two sample T2-test. Results:

> before
         [,1]     [,2]     [,3]     [,4]     [,5]     [,6]     [,7]     [,8]
[1,] 296754.3 392887.7 382483.0 353751.5 343081.1 309991.8 318250.9 300580.9
[2,] 296754.3 366243.3 341282.5 323531.8 306480.8 285785.9 292053.4 276674.9
[3,] 296705.3 392951.7 385432.5 353644.1 336362.4 310969.8 319015.3 296072.7
[4,] 296559.3 360175.8 337877.8 315502.5 304313.5 284135.1 293304.1 269245.2
[5,] 296608.3 359193.8 341406.7 315160.1 310590.8 284253.8 294016.6 272979.4
[6,] 296705.3 366406.8 345990.8 319337.4 308031.1 286945.8 300644.3 278676.6
[7,] 296705.3 368746.6 349376.5 322866.8 314368.8 289354.2 303035.2 280065.5
> after
         [,1]     [,2]     [,3]     [,4]     [,5]     [,6]     [,7]     [,8]
[1,] 296803.3 392951.7 403687.6 370572.7 356478.5 328659.1 331228.2 319236.5
[2,] 296901.3 392977.4 396118.3 363969.7 346094.3 314784.9 327310.9 304212.7
[3,] 296754.3 362377.0 342754.0 317282.7 310844.7 285067.1 291585.0 269960.2
[4,] 296705.3 393080.7 393195.4 360233.3 343581.2 318895.1 324612.5 303263.6
[5,] 296754.3 362493.8 341118.0 315985.0 305509.3 283869.0 297142.8 275491.8
[6,] 296754.3 392887.7 399536.5 366483.8 347971.4 319653.3 327370.7 308022.3

	Hotelling's two sample T2-test

data:  before and after
T.2 = 1.992, df1 = 8, df2 = 4, p-value = 0.264
alternative hypothesis: true location difference is not equal to c(0,0,0,0,0,0,0,0)

Therefore we can not reject that the changes do not have statistically significant impact. For illustration I have created a chart (first two measurements have been discarded, green=v1.99.1, red=HEAD of packet_improvement) here: http://aule.elfove.cz/~brill/pktimpr.png .

This code can't be evaluated by replaying simple queries, as:

• The code uses allocation from packet mempool (which is preallocated, and probably enough for simple queries - likely no heap allocation occurs)
• The code changes parsing of records in the packet, but there are none in the queries (except OPT/TSIG)

The code can be evaluated by benchmarking:

• Parsing of responses to internal events
• Parsing of incoming updates/transfers (this is where it is going to show memory usage spikes)
• Parsing of responses in resolver

@thlavacek Just for the reference, the stuff from @mvavrusa is probably not something to do right now, but we should probably think about it in the long term. The bigger the Knot DNS deployment is the bigger trust in the code changes we must have (@yoda).

Missing return check (can be negative!). Please, add missing check also in knot_rrset_rr_to_canonical (rrset.c) and change 'size_t' type of 'size' variable to 'int' in knot_naptr_header_size (naptr.h).

Fixed.

Added 2 commits:

• 419be1b6 - Changed the type to fit the return value type.
• f2020ef6 - Added return value checks.

Added 12 commits:

• fb585d26 - Treat 0 returned from knot_dname_wire_check() as an error.
• c40ee6f8 - Fix indentation.
• 3d407d28 - Fix indentation.
• 6f301e5b - Initial rdataset sort impl.
• a9ec2cd9 - Initial rdataset reserve impl.
• bc308149 - Add tests for knot_rdataset_reserve() and knot_rdataset_sort_at().
• 787e01d4 - Fix memory leak in the rdataset test.
• 30c3a7a2 - Minor cleanup.
• 21b57873 - Set size of the reserved space.
• 08f9c97f - knot_rdataset_reserve takes the size of rdata without the offsets (header).
• 27dc2b20 - Issue 347 fixed, knot_rdataset_reserve() takes it into account.
• 65f7627f - Skip all buffering when parsing rdata in packet parsing, reserve space in the rdataset and use it.

Added 2 commits:

• 3cb4249e - Typo.
• 6681841e - Unreserve reserved space when error occurs in parse_rdata().

Missing check if *src_avail >= ret

Is returning KNOT_EMALF when *src_avail < ret appropriate?

I think so.

Fixed in a65f4640.

Hm. I've been thinking hard about doing this without the sort (and without the on-stack buffer in knot_rdataset_sort_at). It would be complicated but not that much. Let's go your solution for the moment.

According to @mvavrusa, this approach is just fine since the sort would be a NOP in 99% of the cases.

Oh, really? Maybe for the authoritative serer part. But as I said, this is fine.

For any part, the packet parses each record into a separate rrset and doesn't merge them. https://gitlab.labs.nic.cz/labs/knot/blob/f2020ef69e189b619bbbe8f418226e1aef123383/src/libknot/packet/rrset-wire.h#L53

@jvcelak just keep in mind that I shall remember all your comments on my code when I'm doing another review of your code... ;-)

@dtaborsky The first steps are rough. We will be hard on you. Code quality .

Added 5 commits:

• a65f4640 - rrset_wire: missing size check.
• c8b08158 - rrset_wire: two semicolons.
• 6071a1a9 - rdataset: minor code cleanup.
• 15973d4a - rdataset: minor code cleanup, take two.
• 0a30d3c7 - rrset_wire: assert no trailing data.

Added 2 commits:

• 510b4d03 - rrset_wire: rdata_len returns size instead of using a parameter.
• bcf67b72 - rrset_wire: make trailing data check earlier.

mentioned in commit 9f059eaa

Status changed to merged