add fill_tape_padded to skip the internal input copy - #472
ranflarion wants to merge 1 commit into
Conversation
Licenser
left a comment
Licenser
left a comment
There was a problem hiding this comment.
This largely looks good, I like the option to get moar speedddd! from it but there are a few things to adress
| /// # Safety | ||
| /// | ||
| /// The caller must guarantee `s.len() >= len + INPUT_PADDING`. Like [`fill_tape`], string | ||
| /// unescaping writes in place within the logical input. |
There was a problem hiding this comment.
Instead of a # Safety comment pushing the consideration to the user would it make sense to:
- check that
s.len() >= len + INPUT_PADDINGholds or error - push a
' 'to s.len() to ensure the string is properly terminated
| // Spaces, not zeros: a root-level number or atom is terminated by the byte after it, | ||
| // and a NUL is not a valid terminator. | ||
| scratch.resize(scratch.len() + INPUT_PADDING, b' '); |
There was a problem hiding this comment.
Every test pads with b' ', this is not a guarantee / part of the contract as far as I can tell, the suggestion for #SAfety above would resolve this
| fn invalid_documents_match_fill_tape() { | ||
| assert_matches_fill_tape(&[ | ||
| br#"{"a":"bad\qescape"}"#, | ||
| br#"{"a":"\ud83d"}"#, // unpaired surrogate |
There was a problem hiding this comment.
This document probably shouldn't pass there is #481 fixing that, we probably shouldn't enshrine a bug in the tests 😅
| let mut b = unsafe { src.add(src_i).read() }; | ||
|
|
||
| // quickly skip all the "good stuff" | ||
| while b != b'"' && b != b'\\' { | ||
| src_i += 1; | ||
| b = unsafe { *src.get_kinda_unchecked(src_i) }; | ||
| b = unsafe { src.add(src_i).read() }; |
There was a problem hiding this comment.
we use get_kinda_unchecked to toggle between 'I trust this' reads in release builds and bounds checks in dev for easier error finding I don't think we should change that
2 participants
First half of #469: the padded entry point, without the UTF-8 validation skip (independent, can follow separately).
fill_tape_padded(s, len, buffers, tape)plusINPUT_PADDING. Callers that control the input layout supply the SIMD over-read padding themselves, skipping the O(len) copyfill_tapemakes into its internal buffer: +2% to +7% on the shapes measured in #469, growing with document size.With that copy gone stage 2 reads the same buffer string unescaping writes into, so the read side can no longer be a
&[u8]argument (argument protectors under both borrow models, andnoalias readonly). It becomesInputView, apub(crate)pointer+len threaded throughbuild_tapeand the per-ISAparse_str, withbuild_tapetaking*mut u8for the write side.fill_tapestill passes a disjoint copy, so that path is unchanged.One correction to the issue: since 011d699 dropped NUL from the structural-or-whitespace tables the padding is not arbitrary,
s[len]has to be structural or whitespace (b' 'works). Documented, androot_scalars_terminated_by_paddingcovers it.tests/fill_tape_padded.rspins the result againstfill_tapenode-for-node over lane-boundary escape positions, escapes landing against the padding, shared multi-row scratches, error paths and a random corpus. Miri-clean under Stacked and Tree Borrows on both aarch64 and x86_64, which may be useful for #446/#450; full suite, feature matrix, clippy and fmt green.Happy to rename things or keep
InputViewout ofparse_str's signature if you'd prefer.