-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement QName compression in minmdns #12445
Merged
Merged
Conversation
This file contains 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
…es yet nor functionality
pullapprove
bot
requested review from
anush-apple,
balducci-apple,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
erjiaqing,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
LuDuda,
lzgrablic02 and
msandstedt
December 1, 2021 22:40
PR #12445: Size comparison from dc10c82 to 9a98ea1 Increases above 0.2%:
Increases (11 builds for linux, p6)
Decreases (3 builds for p6)
Full report (22 builds for efr32, k32w, linux, p6, qpg, telink)
|
…lidating (for qname compression support)
PR #12445: Size comparison from dc10c82 to 623b5e3 Increases above 0.2%:
Increases (11 builds for linux, p6)
Decreases (3 builds for p6)
Full report (22 builds for efr32, k32w, linux, p6, qpg, telink)
|
tcarmelveilleux
approved these changes
Dec 2, 2021
PR #12445: Size comparison from 803f3b8 to cb73e42 Increases above 0.2%:
Increases (17 builds for esp32, linux, mbed, p6)
Decreases (6 builds for mbed, p6)
Full report (39 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
woody-apple
approved these changes
Dec 2, 2021
PR #12445: Size comparison from f847731 to bc38e47 Increases above 0.2%:
Increases (17 builds for esp32, linux, mbed, p6)
Decreases (6 builds for mbed, p6)
Full report (39 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
woody-apple
approved these changes
Dec 3, 2021
Moved to fast track: PR up for quite some time, several checkmarks, significant test coverage. |
billwatersiii
pushed a commit
to billwatersiii/connectedhomeip
that referenced
this pull request
Dec 3, 2021
* Revamp constants: qname length constant is NOT used * First pass on an indirection to use a dedicated writer. No test updates yet nor functionality * Updates to use the record writer, more tests pass now * Compressed writer with unit tests * Fix memory leak in UDP responders when a multicast join fails * Add a harder unit test * Make the complex reuse also validate that we can write things other than qnames * Compression applied on all answers (no queries yet) * Prepare to make query writing do compression as well, add more unit tests to qname writing * Record compression for writing serialized qnames as well. Tested that query compression on replies is done * Fix unit tests for resource records for writing QName compression * Code review comments, fix android compile * Fix test advertiser to consider the entire valid packet range when validating (for qname compression support) * Only remember non-fully compressed qnames * Address some code review comments * Add a fit validation for response building * update the code to have RecordWriter be more transparent/easier to use in put statements
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Our MDNS packets begin to be quite large given advertising of vid/ping and multiple IP addresses.
Change overview
Adds support for qname compression when building mdns packets. Expect to make packets somewhat smaller.
Testing
E2E unit tests on linux exercise minmdns a lot
Unit tests updated & added new unit test for the compressed writer
Manual validation with minmdns example that query works