rec: store authority recs and signatures as shared pointers to const data #14985

omoerbeek · 2024-12-18T09:17:36Z

Short description

Currently, we stored and return vectors, change this to store and return a shared pointer to const vectors. This allows us to return a single shared pointer on get(), avoiding to copy all vector elements.

This approach does not work for the answer records, as modified (e.g. TTL) DNSRecords are returned.

Speedup of get() of about 10% in a totally artificial test case of a single insert and many gets(). Should also save a bit on memory. We store nullptr for empty vectors and make sure get() rerurns a ready-made empty vector in that case.

This not only needs review on the detail level, but also on the general approach!

Checklist

I have:

read the CONTRIBUTING.md document
[X compiled this code
tested this code
included documentation (including possible behaviour changes)
documented the code
added or modified regression test(s)
added or modified unit test(s)

…c of shared pointers to DNSRecs

This shows a modest increase in speed: 'null test' 0.11 seconds: 3222473473.3 runs/s, 0.00 us/run 'AuthRecordsTest' 0.10 seconds: 66.8 runs/s, 14977.88 us/run to 'null test' 0.10 seconds: 3223627246.4 runs/s, 0.00 us/run 'AuthRecordsTest' 0.11 seconds: 72.1 runs/s, 13868.26 us/run The test is: generate 100 records into the cache (each with an authRecord), retrieve each record 100 times.

coveralls · 2024-12-18T09:45:58Z

Pull Request Test Coverage Report for Build 12631365029

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

131 of 136 (96.32%) changed or added relevant lines in 9 files are covered.
9097 unchanged lines in 174 files lost coverage.
Overall coverage increased (+8.2%) to 64.817%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pdns/recursordist/syncres.cc	25	26	96.15%
pdns/recursordist/recursor_cache.cc	55	57	96.49%
pdns/recursordist/test-syncres_cc.cc	18	20	90.0%

Files with Coverage Reduction	New Missed Lines	%
pdns/dnsdistdist/dnsdist-session-cache.cc	1	62.86%
pdns/auth-catalogzone.hh	1	66.67%
pdns/dnsdistdist/test-dnsdistrules_cc.cc	1	95.15%
pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc	1	85.12%
pdns/recursordist/nod.hh	1	92.59%
ext/lmdb-safe/lmdb-typed.cc	1	89.66%
pdns/ws-auth.cc	1	80.9%
pdns/test-dnsrecords_cc.cc	2	95.97%
pdns/dnsdistdist/test-dnsdist-lua-ffi.cc	2	99.59%
pdns/epollmplexer.cc	2	85.0%

Totals
Change from base Build 12421459958:	8.2%
Covered Lines:	126127
Relevant Lines:	163803

💛 - Coveralls

rgacogne · 2024-12-24T09:22:20Z

pdns/recursordist/recursor_cache.cc

+      *signatures = std::make_shared<SigRecsVec>(vec);
+    }
+    else {
+      assert(*signatures == nullptr || (*signatures)->empty());


This feels wrong. If the condition above is not correct, we could get here with some entries in *signatures as long as entry->d_signatures doesn't contain any, right?

Right, the assert seems too strict.

rgacogne · 2024-12-24T09:26:00Z

pdns/recursordist/recursor_cache.cc

+      // Return a new vec if we need to append to a non-empty vector
+      SigRecsVec vec(**signatures);
+      vec.insert(vec.end(), entry->d_signatures->cbegin(), entry->d_signatures->cend());
+      *signatures = std::make_shared<SigRecsVec>(vec);


Would it make sense to create the shared pointer right away and insert into it? Or at least to move vec?

rgacogne · 2024-12-24T09:28:15Z

pdns/recursordist/recursor_cache.hh

+  // pointer gets copied, while earlier code would copy all shared pointer in the vector.
+  //
+  // In the current SyncRes code, AuthRecs never get appended to a non-empty vector while SigRecs do
+  // get appended in some cases; the handleHit() code will take measures. In the futrue we might


Suggested change

// get appended in some cases; the handleHit() code will take measures. In the futrue we might

// get appended in some cases; the handleHit() code will take measures. In the future we might

rgacogne · 2024-12-24T09:28:24Z

pdns/recursordist/recursor_cache.hh

+  //
+  // In the current SyncRes code, AuthRecs never get appended to a non-empty vector while SigRecs do
+  // get appended in some cases; the handleHit() code will take measures. In the futrue we might
+  // want a more specialized datastructure than a vector, it would require another level of


Suggested change

// want a more specialized datastructure than a vector, it would require another level of

// want a more specialized data structure than a vector, it would require another level of

rgacogne · 2024-12-24T09:30:36Z

pdns/recursordist/recursor_cache.cc

@@ -1016,18 +1042,18 @@ void MemRecursorCache::getRecordSet(T& message, U recordSet)
  for (const auto& record : recordSet->d_records) {
    message.add_bytes(PBCacheEntry::repeated_bytes_record, record->serialize(recordSet->d_qname, true));
  }
-  for (const auto& record : recordSet->d_signatures) {
+  for (const auto& record : *recordSet->d_signatures) {


I think we need to check that d_signatures is not nullptr first.

rgacogne · 2024-12-24T09:30:42Z

pdns/recursordist/recursor_cache.cc

    message.add_bytes(PBCacheEntry::repeated_bytes_sig, record->serialize(recordSet->d_qname, true));
  }
-  for (const auto& authRec : recordSet->d_authorityRecs) {
+  for (const auto& authRec : *recordSet->d_authorityRecs) {


rgacogne · 2024-12-24T09:32:22Z

pdns/recursordist/recursor_cache.cc

+      if (!cacheEntry.d_signatures) {
+        cacheEntry.d_signatures = std::make_shared<SigRecsVec>();
+      }


I think this can go?

rgacogne · 2024-12-24T09:32:30Z

pdns/recursordist/recursor_cache.cc

+      if (!cacheEntry.d_authorityRecs) {
+        cacheEntry.d_authorityRecs = std::make_shared<AuthRecsVec>();
+      }


rgacogne · 2024-12-24T09:39:16Z

pdns/recursordist/recursor_cache.hh

+  // store a shared pointer to the copied data. The shared pointer will be returned by get().  There
+  // are optimizations: an empty vector will be stored as a nullptr, but get() will return a pointer
+  // to an already existing empty vector in that case, this is more convenient for the caller, since
+  // it avoid checking for nullptr, just iterate as for the non-empty case.


I wonder how the optimization holds when we have a lot of threads: we avoid allocating memory which is good (but could be done in a thread-specific pool without any contention) at the cost of having (in some cases, which may be rare?) several threads atomically accessing and updating the same memory location (the shared reference counter). I guess the optimization is worth it if we do not cross NUMA-boundaries, and perhaps a bit less if we do?

Good point, when I was writing this I was wondering this as well. Many new/delete implementation use some form of locking as several threads share the same memory pool, so I was guessing it would be beneficial in the typical case. I do not have hard data, though.

I'm OK with ignoring this unless/until it shows up in a performance profile :)

rgacogne

👍

…be passed a nullptr

rec: followup to #14985: init shared pointers as get() might be passed a nullptr

omoerbeek added 6 commits December 18, 2024 09:55

rec: experiment: authRecs a shared pointer of DNSRecs instead of a ve…

75951ec

…c of shared pointers to DNSRecs

Improvement: use const vector plus names for the various types used

d8e16f3

clang-tidy

3e00b6a

Also handle sigs as a shared point to a vector

45e3241

also test sigs

4a2ec31

omoerbeek added rec enhancement labels Dec 18, 2024

omoerbeek added this to the rec-5.3.0 milestone Dec 18, 2024

rgacogne requested changes Dec 24, 2024

View reviewed changes

Process review comments from @rgacogne

d5a9bdb

omoerbeek requested a review from rgacogne January 6, 2025 11:05

rgacogne approved these changes Jan 6, 2025

View reviewed changes

omoerbeek merged commit 60cfbea into PowerDNS:master Jan 6, 2025
77 checks passed

omoerbeek deleted the rec-cache-shared-authrecs branch January 6, 2025 13:02

omoerbeek added a commit to omoerbeek/pdns that referenced this pull request Jan 7, 2025

rec: followup to PowerDNS#14985: init shared pointers as get() might …

2990eba

…be passed a nullptr

omoerbeek mentioned this pull request Jan 7, 2025

rec: followup to #14985: init shared pointers as get() might be passed a nullptr #15017

Merged

7 tasks

omoerbeek added a commit that referenced this pull request Jan 7, 2025

Merge pull request #15017 from omoerbeek/rec-shared-ptr-followup

a9b1ba5

rec: followup to #14985: init shared pointers as get() might be passed a nullptr

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rec: store authority recs and signatures as shared pointers to const data #14985

rec: store authority recs and signatures as shared pointers to const data #14985

omoerbeek commented Dec 18, 2024

coveralls commented Dec 18, 2024 •

edited

Loading

rgacogne Dec 24, 2024

omoerbeek Jan 6, 2025

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

omoerbeek Jan 6, 2025

rgacogne Jan 6, 2025

rgacogne left a comment

	// get appended in some cases; the handleHit() code will take measures. In the futrue we might
	// get appended in some cases; the handleHit() code will take measures. In the future we might

	// want a more specialized datastructure than a vector, it would require another level of
	// want a more specialized data structure than a vector, it would require another level of

rec: store authority recs and signatures as shared pointers to const data #14985

rec: store authority recs and signatures as shared pointers to const data #14985

Conversation

omoerbeek commented Dec 18, 2024

Short description

Checklist

coveralls commented Dec 18, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12631365029

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

rgacogne left a comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2024 •

edited

Loading