Skip to content

Modernization: Implement base data-structures and replace usage#540

Merged
bradh352 merged 37 commits into
c-ares:mainfrom
bradh352:llist
Aug 14, 2023
Merged

Modernization: Implement base data-structures and replace usage#540
bradh352 merged 37 commits into
c-ares:mainfrom
bradh352:llist

Conversation

@bradh352

@bradh352 bradh352 commented Aug 10, 2023

Copy link
Copy Markdown
Member

c-ares currently lacks modern data structures that can make coding easier and more efficient. This PR implements a new linked list, skip list (sorted linked list), and hashtable implementation that are easy to use and hard to misuse. Though these implementations use more memory allocations than the prior implementation, the ability to more rapidly iterate on the codebase is a bigger win than any marginal performance difference (which is unlikely to be visible, modern systems are much more powerful than when c-ares was initially created).

The data structure implementation favors readability and audit-ability over performance, however using the algorithmically correct data type for the purpose should offset any perceived losses.

The primary motivation for this PR is to facilitate future implementation for Issues #444, #135, #458, and possibly #301

A couple additional notes:

  • The ares_timeout() function is now O(1) complexity instead of O(n) due to the use of a skiplist.
  • Some obscure bugs were uncovered which were actually being incorrectly validated in the test cases. These have been addressed in this PR but are not explicitly discussed.
  • Fixed some dead code warnings in ares_rand for systems that don't need rc4

@bradh352
bradh352 requested a review from bagder August 11, 2023 00:27
@bradh352

Copy link
Copy Markdown
Member Author

FYI, windows build failed due to whitespace issues in Makefile.inc that I'll fix. The darwin build issue I've researched extensively, it appears to be something related to some DNS server in Cirrus-CI not being able to handle large responses so it times out then returns a server failure. It all depends on which Cirrus-CI node you hit. At first I thought it was my issue, but that's how I uncovered some bugs in what response code c-ares was returning and this PR fixes that too.

… ares__rand caches internally. add note in htable why we're not using ares__slist_t for chaining on collisions
@bradh352

Copy link
Copy Markdown
Member Author

@bagder don't suppose you can review today? If not, I'll probably just go ahead and merge it so I can start work on the tickets referenced in the description. I can't really imagine there's anything objectionable here.

@bagder bagder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bradh352
bradh352 merged commit cf99c02 into c-ares:main Aug 14, 2023
@bradh352
bradh352 deleted the llist branch August 14, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants