Prevent NONEEDEDGLUE errors and add cache statistics#456
Merged
phillip-stephens merged 22 commits intomainfrom Sep 30, 2024
Merged
Prevent NONEEDEDGLUE errors and add cache statistics#456phillip-stephens merged 22 commits intomainfrom
phillip-stephens merged 22 commits intomainfrom
Conversation
… failures, which is par for the course
zakird
reviewed
Sep 28, 2024
| close(outChan) | ||
| close(metaChan) | ||
| routineWG.Wait() | ||
| resolverConfig.Cache.Stats.PrintStatistics() |
zakird
approved these changes
Sep 28, 2024
…t cache stats at verbosty=5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Prior to this change, we were adding
AuthoritiesandAdditionalsas separate records in the cache. This meant that theAuthoritieswas inserted as an NS record, but the Additionals were inserted as separate A/AAAA records. Separately, our cache is an LRU cache with 4096 shards, 2 entries per shard by default. What this means is that we have no guarantees that all of a domain's Authorities + Additionals are in the cache, since a partial set can be evicted.This loss of accuracy was what caused the
NONEEDEDGLUErecords to pop up, especially as more domains in the same run.Changes
--verbosity=5. No performance impact if--verbosity < 5time="2024-09-27T20:28:52Z" level=debug msg="Cache statistics: hits=29870 misses=85581 adds=30790 ejects=21604 hitRate=25.872448% missRate=74.127552%"SingleQueryResultpointer vs. returning the struct since each function would have to create a new structure and copy the internals. The pointer avoids this.Authorityrecord or `Answer record are stored atomicallyAuthorityrecords store the authority's for a certain domain -.comAnswerrecords store the authoritative answers for a certain query -A google.comPerformance
Tested with 10000 top domains,
A --iterative --threads=100 --verbosity=5main
Runtime - 37.2 s
Allocs - 2199 MB total allocations (this is not peak memory use since the Garbage Collector cleans up un-used memory, this is just total allocated objects)
NOERROR - 9919
TIMEOUT/ITERATIVE_TIMEOUT - 5
NXDOMAIN - 54
Branch
Runtime - 33.9 s
Allocs - 1216 MB
NOERROR - 9916
TIMEOUT/ITERATIVE_TIMEOUT - 9
NXDOMAIN - 55
Main takeaway is there was a sharp reduction in allocations, however the runtime reduction wasn't that large (and likely within the margin of error, other A/B tests didn't show such a large delta), so it seems garbage collection is not our bottleneck.
Either way, I think this is a needed change for cache use.