config: support backslash-newline line continuation (#611) - #625
Mobeen0119 wants to merge 1 commit into
Conversation
|
This is the third time you have created this PR. Can you please just amend/rebase new changes and force push them instead of recreating the PR everytime? |
|
I would suggest improving performance when It should skip lines that don't contain kernel command lines instead of checking every single line. |
|
This pipeline already runs multiple full-buffer passes before mine (hash check, continuation-collapse, whitespace trim, macro load, macro expand), so mine isn't adding a new category of cost and skipping non-cmdline lines isn't possible at this point since nothing's tokenized into lines yet. Worth noticing: the whitespace-trim loop right after mine is actually O(n²), not O(n) ... it shifts the entire remaining buffer on every skip event, which is the bigger perf risk on large configs, and it's pre-existing. |
cbba523 to
4cc83d7
Compare
hotline1337
left a comment
There was a problem hiding this comment.
This pipeline already runs multiple full-buffer passes before mine (hash check, continuation-collapse, whitespace trim, macro load, macro expand), so mine isn't adding a new category of cost and skipping non-cmdline lines isn't possible at this point since nothing's tokenized into lines yet. Worth noticing: the whitespace-trim loop right after mine is actually O(n²), not O(n) ... it shifts the entire remaining buffer on every skip event, which is the bigger perf risk on large configs, and it's pre-existing.
Since you already identified the real performance concern - it might be worth fixing it in the same PR. Right now it shifts the entire remaining buffer on every skip event:
if (skip) {
for (size_t j = i; j < config_size - skip; j++)
config_addr[j] = config_addr[j + skip];
config_size -= skip;
i--;
}With k skip events this is O(n*k), worst case O(n^2) (e.g. a config full of trailing spaces).
Here's a drop-in O(n) replacement using the same read/write pointer pattern as your continuation pass:
// Remove carriage returns and leading/trailing whitespace from lines
{
size_t write = 0;
bool at_line_start = true;
for (size_t read = 0; read < config_size; read++) {
char c = config_addr[read];
if (c == '\r') {
continue;
}
if (at_line_start && (c == ' ' || c == '\t')) {
continue;
}
if (c == '\n') {
while (write > 0 && (config_addr[write - 1] == ' ' || config_addr[write - 1] == '\t')) {
write--;
}
at_line_start = true;
} else {
at_line_start = false;
}
config_addr[write++] = c;
}
config_size = write;
}Same semantics: strips \r, strips leading whitespace after \n, strips trailing whitespace before \n. The trailing-whitespace backtrack is amortised O(1) per character (each byte is "backed over" at most once). Single pass, no buffer shifts.
|
old code kept trailing spaces before \r\n but stripped them before \n which is inconsistent. this code strips them the same way either time, which is correct. Tested against 200k fuzz inputs + 30 edge cases to confirm this is the only real difference from the old behavior. wanted to call it out since it's a behavior change, not just a speed fix, before merging. |
4cc83d7 to
947d9b5
Compare
There was a problem hiding this comment.
Move config_alloc_size = config_size; to before the continuation pass (i.e., right after L#411, before L#414), otherwise pmm_free(config_addr, config_alloc_size) will be called with the wrong size whenever the config uses line continuation at all, which is the entire point of this feature.
947d9b5 to
f48624e
Compare
…tloader#611) Also fixes O(n^2) whitespace-trim loop to O(n).... now strips trailing whitespace before CRLF too, verified with 200k fuzz tests.
f48624e to
8614105
Compare
hotline1337
left a comment
There was a problem hiding this comment.
Good Job! Now it's up to the maintainers if this gets approved to be merged. Thank you for fixing and improving the code!
Add backslash-newline line continuation to config parser.
O(n) two-pointer approach.
Handles Unix (\n) and Windows (\r\n) line endings.
Example:
CMDLINE: root=/dev/sda1 rw quiet
zswap.enabled=0
Becomes one continuous line.
Tested with 20 unit tests + 100K fuzz + 50K comparisons vs old code.
Fixes #611
Test Files :
test_compare.c
test_full.c