From 0b0350b6f099f1d5a0b6b764a0264f13cb60d5e7 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 10 Sep 2024 07:53:52 +0100 Subject: [PATCH 1/6] iterative algorithm for take_value --- crates/jiter/src/value.rs | 199 ++++++++++++++++++++++++++++++-------- 1 file changed, 157 insertions(+), 42 deletions(-) diff --git a/crates/jiter/src/value.rs b/crates/jiter/src/value.rs index 78f3e024..5254e3c2 100644 --- a/crates/jiter/src/value.rs +++ b/crates/jiter/src/value.rs @@ -99,19 +99,6 @@ impl JsonValue<'static> { } } -macro_rules! check_recursion { - ($recursion_limit:ident, $index:expr, $($body:tt)*) => { - $recursion_limit = match $recursion_limit.checked_sub(1) { - Some(limit) => limit, - None => return crate::errors::json_err!(RecursionLimitExceeded, $index), - }; - - $($body)* - - $recursion_limit += 1; - }; -} - pub(crate) fn take_value_borrowed<'j>( peek: Peek, parser: &mut Parser<'j>, @@ -150,7 +137,7 @@ fn take_value<'j, 's>( peek: Peek, parser: &mut Parser<'j>, tape: &mut Tape, - mut recursion_limit: u8, + recursion_limit: u8, allow_inf_nan: bool, create_cow: &impl Fn(StringOutput<'_, 'j>) -> Cow<'s, str>, ) -> JsonResult> { @@ -173,42 +160,41 @@ fn take_value<'j, 's>( } Peek::Array => { // we could do something clever about guessing the size of the array - let mut array: SmallVec<[JsonValue<'s>; 8]> = SmallVec::new(); + let array: SmallVec<[JsonValue<'s>; 8]> = SmallVec::new(); if let Some(peek_first) = parser.array_first()? { - check_recursion!(recursion_limit, parser.index, - let v = take_value(peek_first, parser, tape, recursion_limit, allow_inf_nan, create_cow)?; - ); - array.push(v); - while let Some(peek) = parser.array_step()? { - check_recursion!(recursion_limit, parser.index, - let v = take_value(peek, parser, tape, recursion_limit, allow_inf_nan, create_cow)?; - ); - array.push(v); - } + take_value_recursive( + peek_first, + RecursedValue::Array(array), + parser, + tape, + recursion_limit, + allow_inf_nan, + create_cow, + ) + } else { + Ok(JsonValue::Array(Arc::new(array))) } - Ok(JsonValue::Array(Arc::new(array))) } Peek::Object => { // same for objects - let mut object: LazyIndexMap, JsonValue<'s>> = LazyIndexMap::new(); + let object: LazyIndexMap, JsonValue<'s>> = LazyIndexMap::new(); if let Some(first_key) = parser.object_first::(tape)? { let first_key = create_cow(first_key); - let peek = parser.peek()?; - check_recursion!(recursion_limit, parser.index, - let first_value = take_value(peek, parser, tape, recursion_limit, allow_inf_nan, create_cow)?; - ); - object.insert(first_key, first_value); - while let Some(key) = parser.object_step::(tape)? { - let key = create_cow(key); - let peek = parser.peek()?; - check_recursion!(recursion_limit, parser.index, - let value = take_value(peek, parser, tape, recursion_limit, allow_inf_nan, create_cow)?; - ); - object.insert(key, value); - } + take_value_recursive( + parser.peek()?, + RecursedValue::Object { + partial: object, + next_key: first_key, + }, + parser, + tape, + recursion_limit, + allow_inf_nan, + create_cow, + ) + } else { + Ok(JsonValue::Object(Arc::new(object))) } - - Ok(JsonValue::Object(Arc::new(object))) } _ => { let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); @@ -228,6 +214,135 @@ fn take_value<'j, 's>( } } +enum RecursedValue<'s> { + Array(SmallVec<[JsonValue<'s>; 8]>), + Object { + partial: LazyIndexMap, JsonValue<'s>>, + next_key: Cow<'s, str>, + }, +} + +#[inline(never)] // this is an iterative algo called only from take_value, no point in inlining +fn take_value_recursive<'j, 's>( + mut peek: Peek, + mut current_recursion: RecursedValue<'s>, + parser: &mut Parser<'j>, + tape: &mut Tape, + recursion_limit: u8, + allow_inf_nan: bool, + create_cow: &impl Fn(StringOutput<'_, 'j>) -> Cow<'s, str>, +) -> JsonResult> { + let mut recursion_stack = SmallVec::<[RecursedValue<'_>; 8]>::new(); + let recursion_limit: usize = recursion_limit.into(); + + macro_rules! push_recursion { + ($next_peek:expr, $value:expr) => { + peek = $next_peek; + recursion_stack.push(std::mem::replace(&mut current_recursion, $value)); + if recursion_stack.len() >= recursion_limit { + return Err(json_error!(RecursionLimitExceeded, parser.index)); + } + }; + } + + loop { + let mut value = match peek { + Peek::True => { + parser.consume_true()?; + JsonValue::Bool(true) + } + Peek::False => { + parser.consume_false()?; + JsonValue::Bool(false) + } + Peek::Null => { + parser.consume_null()?; + JsonValue::Null + } + Peek::String => { + let s = parser.consume_string::(tape, false)?; + JsonValue::Str(create_cow(s)) + } + Peek::Array => { + let array = SmallVec::new(); + if let Some(next_peek) = parser.array_first()? { + push_recursion!(next_peek, RecursedValue::Array(array)); + // immediately jump to process the first value in the array + continue; + } else { + JsonValue::Array(Arc::new(array)) + } + } + Peek::Object => { + let object = LazyIndexMap::new(); + if let Some(next_key) = parser.object_first::(tape)? { + push_recursion!( + parser.peek()?, + RecursedValue::Object { + partial: object, + next_key: create_cow(next_key) + } + ); + continue; + } else { + JsonValue::Object(Arc::new(object)) + } + } + _ => { + let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); + match n { + Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), + Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), + Ok(NumberAny::Float(float)) => JsonValue::Float(float), + Err(e) => { + if !peek.is_num() { + return Err(json_error!(ExpectedSomeValue, parser.index)); + } else { + return Err(e); + } + } + } + } + }; + + // now try to advance position in the current array or object + peek = loop { + current_recursion = match current_recursion { + RecursedValue::Array(mut array) => { + array.push(value); + if let Some(next_peek) = parser.array_step()? { + // array continuing + current_recursion = RecursedValue::Array(array); + break next_peek; + } else if let Some(recursed) = recursion_stack.pop() { + // array finished, recursing + value = JsonValue::Array(Arc::new(array)); + recursed + } else { + // no recursion left and array finished + return Ok(JsonValue::Array(Arc::new(array))); + } + } + RecursedValue::Object { mut partial, next_key } => { + partial.insert(next_key, value); + if let Some(next_key) = parser.object_step::(tape)?.map(create_cow) { + // object continuing + current_recursion = RecursedValue::Object { partial, next_key }; + break parser.peek()?; + } else if let Some(recursed) = recursion_stack.pop() { + // object finished, recursing + value = JsonValue::Object(Arc::new(LazyIndexMap::new())); + recursed + } else { + // no recursion left and object finished + return Ok(JsonValue::Object(Arc::new(partial))); + } + } + } + }; + } +} + /// like `take_value`, but nothing is returned, should be faster than `take_value`, useful when you don't care /// about the value, but just want to consume it pub(crate) fn take_value_skip( From ebd407e19234b0fbb8626cb298b099ce4bcd0c1b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 10 Sep 2024 08:20:05 +0100 Subject: [PATCH 2/6] try to avoid lots of mem moves --- crates/jiter/src/value.rs | 46 ++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/crates/jiter/src/value.rs b/crates/jiter/src/value.rs index 5254e3c2..eaca36f1 100644 --- a/crates/jiter/src/value.rs +++ b/crates/jiter/src/value.rs @@ -307,38 +307,40 @@ fn take_value_recursive<'j, 's>( // now try to advance position in the current array or object peek = loop { - current_recursion = match current_recursion { - RecursedValue::Array(mut array) => { - array.push(value); + match &mut current_recursion { + RecursedValue::Array(array) => { if let Some(next_peek) = parser.array_step()? { + array.push(value); // array continuing - current_recursion = RecursedValue::Array(array); break next_peek; - } else if let Some(recursed) = recursion_stack.pop() { - // array finished, recursing - value = JsonValue::Array(Arc::new(array)); - recursed - } else { - // no recursion left and array finished - return Ok(JsonValue::Array(Arc::new(array))); } } - RecursedValue::Object { mut partial, next_key } => { - partial.insert(next_key, value); - if let Some(next_key) = parser.object_step::(tape)?.map(create_cow) { + RecursedValue::Object { partial, next_key } => { + if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { + partial.insert(std::mem::replace(next_key, yet_another_key), value); // object continuing - current_recursion = RecursedValue::Object { partial, next_key }; break parser.peek()?; - } else if let Some(recursed) = recursion_stack.pop() { - // object finished, recursing - value = JsonValue::Object(Arc::new(LazyIndexMap::new())); - recursed - } else { - // no recursion left and object finished - return Ok(JsonValue::Object(Arc::new(partial))); } } } + + value = match current_recursion { + RecursedValue::Array(mut array) => { + array.push(value); + JsonValue::Array(Arc::new(array)) + } + RecursedValue::Object { mut partial, next_key } => { + partial.insert(next_key, value); + JsonValue::Object(Arc::new(partial)) + } + }; + + if let Some(r) = recursion_stack.pop() { + // value is the current array or object, next turn of the loop will insert it to the parent + current_recursion = r; + } else { + return Ok(value); + } }; } } From fc455a2131badf0442d083b26c488184a79e06a2 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 10 Sep 2024 09:47:36 +0100 Subject: [PATCH 3/6] split array / object loops to be hotter --- crates/jiter/src/value.rs | 243 ++++++++++++++++++++++++++------------ 1 file changed, 169 insertions(+), 74 deletions(-) diff --git a/crates/jiter/src/value.rs b/crates/jiter/src/value.rs index eaca36f1..96762765 100644 --- a/crates/jiter/src/value.rs +++ b/crates/jiter/src/value.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use std::sync::Arc; use num_bigint::BigInt; -use smallvec::SmallVec; +use smallvec::{smallvec, SmallVec}; use crate::errors::{json_error, JsonError, JsonResult, DEFAULT_RECURSION_LIMIT}; use crate::lazy_index_map::LazyIndexMap; @@ -225,121 +225,216 @@ enum RecursedValue<'s> { #[inline(never)] // this is an iterative algo called only from take_value, no point in inlining fn take_value_recursive<'j, 's>( mut peek: Peek, - mut current_recursion: RecursedValue<'s>, + current_recursion: RecursedValue<'s>, parser: &mut Parser<'j>, tape: &mut Tape, recursion_limit: u8, allow_inf_nan: bool, create_cow: &impl Fn(StringOutput<'_, 'j>) -> Cow<'s, str>, ) -> JsonResult> { - let mut recursion_stack = SmallVec::<[RecursedValue<'_>; 8]>::new(); let recursion_limit: usize = recursion_limit.into(); + let mut recursion_stack: SmallVec<[RecursedValue; 8]> = smallvec![current_recursion]; + let mut current_recursion = recursion_stack.last_mut().expect("just inserted one element"); + macro_rules! push_recursion { ($next_peek:expr, $value:expr) => { peek = $next_peek; - recursion_stack.push(std::mem::replace(&mut current_recursion, $value)); if recursion_stack.len() >= recursion_limit { return Err(json_error!(RecursionLimitExceeded, parser.index)); } + recursion_stack.push($value); + current_recursion = recursion_stack.last_mut().expect("just inserted one element"); }; } - loop { - let mut value = match peek { - Peek::True => { - parser.consume_true()?; - JsonValue::Bool(true) - } - Peek::False => { - parser.consume_false()?; - JsonValue::Bool(false) - } - Peek::Null => { - parser.consume_null()?; - JsonValue::Null - } - Peek::String => { - let s = parser.consume_string::(tape, false)?; - JsonValue::Str(create_cow(s)) - } - Peek::Array => { - let array = SmallVec::new(); - if let Some(next_peek) = parser.array_first()? { - push_recursion!(next_peek, RecursedValue::Array(array)); - // immediately jump to process the first value in the array - continue; - } else { - JsonValue::Array(Arc::new(array)) - } - } - Peek::Object => { - let object = LazyIndexMap::new(); - if let Some(next_key) = parser.object_first::(tape)? { - push_recursion!( - parser.peek()?, - RecursedValue::Object { - partial: object, - next_key: create_cow(next_key) + 'recursion: loop { + let mut value = match current_recursion { + RecursedValue::Array(array) => loop { + let value = match peek { + Peek::True => { + parser.consume_true()?; + JsonValue::Bool(true) + } + Peek::False => { + parser.consume_false()?; + JsonValue::Bool(false) + } + Peek::Null => { + parser.consume_null()?; + JsonValue::Null + } + Peek::String => { + let s = parser.consume_string::(tape, false)?; + JsonValue::Str(create_cow(s)) + } + Peek::Array => { + if let Some(next_peek) = parser.array_first()? { + push_recursion!(next_peek, RecursedValue::Array(SmallVec::new())); + // immediately jump to process the first value in the array + continue 'recursion; + } else { + JsonValue::Array(Arc::new(SmallVec::new())) + } + } + Peek::Object => { + let object = LazyIndexMap::new(); + if let Some(next_key) = parser.object_first::(tape)? { + push_recursion!( + parser.peek()?, + RecursedValue::Object { + partial: object, + next_key: create_cow(next_key) + } + ); + continue 'recursion; + } else { + JsonValue::Object(Arc::new(object)) + } + } + _ => { + let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); + match n { + Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), + Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), + Ok(NumberAny::Float(float)) => JsonValue::Float(float), + Err(e) => { + if !peek.is_num() { + return Err(json_error!(ExpectedSomeValue, parser.index)); + } else { + return Err(e); + } + } } - ); + } + }; + + // now try to advance position in the current array + if let Some(next_peek) = parser.array_step()? { + array.push(value); + peek = next_peek; + // array continuing continue; - } else { - JsonValue::Object(Arc::new(object)) } - } - _ => { - let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); - match n { - Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), - Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), - Ok(NumberAny::Float(float)) => JsonValue::Float(float), - Err(e) => { - if !peek.is_num() { - return Err(json_error!(ExpectedSomeValue, parser.index)); + + let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { + unreachable!("known to be in array recursion"); + }; + + array.push(value); + break JsonValue::Array(Arc::new(array)); + }, + RecursedValue::Object { partial, next_key } => loop { + let value = match peek { + Peek::True => { + parser.consume_true()?; + JsonValue::Bool(true) + } + Peek::False => { + parser.consume_false()?; + JsonValue::Bool(false) + } + Peek::Null => { + parser.consume_null()?; + JsonValue::Null + } + Peek::String => { + let s = parser.consume_string::(tape, false)?; + JsonValue::Str(create_cow(s)) + } + Peek::Array => { + let array = SmallVec::new(); + if let Some(next_peek) = parser.array_first()? { + push_recursion!(next_peek, RecursedValue::Array(array)); + // immediately jump to process the first value in the array + continue 'recursion; } else { - return Err(e); + JsonValue::Array(Arc::new(array)) } } + Peek::Object => { + let object = LazyIndexMap::new(); + if let Some(yet_another_key) = parser.object_first::(tape)? { + push_recursion!( + parser.peek()?, + RecursedValue::Object { + partial: object, + next_key: create_cow(yet_another_key) + } + ); + continue 'recursion; + } else { + JsonValue::Object(Arc::new(object)) + } + } + _ => { + let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); + match n { + Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), + Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), + Ok(NumberAny::Float(float)) => JsonValue::Float(float), + Err(e) => { + if !peek.is_num() { + return Err(json_error!(ExpectedSomeValue, parser.index)); + } else { + return Err(e); + } + } + } + } + }; + + // now try to advance position in the current object + if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { + // object continuing + partial.insert(std::mem::replace(next_key, yet_another_key), value); + peek = parser.peek()?; + continue; } - } + + let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { + unreachable!("known to be in object recursion"); + }; + + partial.insert(next_key, value); + break JsonValue::Object(Arc::new(partial)); + }, }; - // now try to advance position in the current array or object + // current array or object has finished; + // try to pop and continue with the parent peek = loop { - match &mut current_recursion { + if let Some(next_recursion) = recursion_stack.last_mut() { + current_recursion = next_recursion; + } else { + return Ok(value); + } + + value = match current_recursion { RecursedValue::Array(array) => { if let Some(next_peek) = parser.array_step()? { array.push(value); - // array continuing break next_peek; } + + let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { + unreachable!("known to be in object recursion"); + }; + array.push(value); + JsonValue::Array(Arc::new(array)) } RecursedValue::Object { partial, next_key } => { if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { partial.insert(std::mem::replace(next_key, yet_another_key), value); - // object continuing break parser.peek()?; } - } - } - value = match current_recursion { - RecursedValue::Array(mut array) => { - array.push(value); - JsonValue::Array(Arc::new(array)) - } - RecursedValue::Object { mut partial, next_key } => { + let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { + unreachable!("known to be in object recursion"); + }; partial.insert(next_key, value); JsonValue::Object(Arc::new(partial)) } - }; - - if let Some(r) = recursion_stack.pop() { - // value is the current array or object, next turn of the loop will insert it to the parent - current_recursion = r; - } else { - return Ok(value); } }; } From f66635c10da4990284b2b87c28e6c064779a69c8 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 10 Sep 2024 10:01:45 +0100 Subject: [PATCH 4/6] try arc-ing inside recursed values --- crates/jiter/src/value.rs | 57 +++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/crates/jiter/src/value.rs b/crates/jiter/src/value.rs index 96762765..bb5b00bf 100644 --- a/crates/jiter/src/value.rs +++ b/crates/jiter/src/value.rs @@ -160,7 +160,7 @@ fn take_value<'j, 's>( } Peek::Array => { // we could do something clever about guessing the size of the array - let array: SmallVec<[JsonValue<'s>; 8]> = SmallVec::new(); + let array = Arc::new(SmallVec::new()); if let Some(peek_first) = parser.array_first()? { take_value_recursive( peek_first, @@ -172,12 +172,12 @@ fn take_value<'j, 's>( create_cow, ) } else { - Ok(JsonValue::Array(Arc::new(array))) + Ok(JsonValue::Array(array)) } } Peek::Object => { // same for objects - let object: LazyIndexMap, JsonValue<'s>> = LazyIndexMap::new(); + let object = Arc::new(LazyIndexMap::new()); if let Some(first_key) = parser.object_first::(tape)? { let first_key = create_cow(first_key); take_value_recursive( @@ -193,7 +193,7 @@ fn take_value<'j, 's>( create_cow, ) } else { - Ok(JsonValue::Object(Arc::new(object))) + Ok(JsonValue::Object(object)) } } _ => { @@ -215,9 +215,9 @@ fn take_value<'j, 's>( } enum RecursedValue<'s> { - Array(SmallVec<[JsonValue<'s>; 8]>), + Array(JsonArray<'s>), Object { - partial: LazyIndexMap, JsonValue<'s>>, + partial: JsonObject<'s>, next_key: Cow<'s, str>, }, } @@ -269,16 +269,17 @@ fn take_value_recursive<'j, 's>( JsonValue::Str(create_cow(s)) } Peek::Array => { + let array = Arc::new(SmallVec::new()); if let Some(next_peek) = parser.array_first()? { - push_recursion!(next_peek, RecursedValue::Array(SmallVec::new())); + push_recursion!(next_peek, RecursedValue::Array(array)); // immediately jump to process the first value in the array continue 'recursion; } else { - JsonValue::Array(Arc::new(SmallVec::new())) + JsonValue::Array(array) } } Peek::Object => { - let object = LazyIndexMap::new(); + let object = Arc::new(LazyIndexMap::new()); if let Some(next_key) = parser.object_first::(tape)? { push_recursion!( parser.peek()?, @@ -289,7 +290,7 @@ fn take_value_recursive<'j, 's>( ); continue 'recursion; } else { - JsonValue::Object(Arc::new(object)) + JsonValue::Object(object) } } _ => { @@ -311,7 +312,7 @@ fn take_value_recursive<'j, 's>( // now try to advance position in the current array if let Some(next_peek) = parser.array_step()? { - array.push(value); + Arc::get_mut(array).expect("sole writer").push(value); peek = next_peek; // array continuing continue; @@ -321,8 +322,8 @@ fn take_value_recursive<'j, 's>( unreachable!("known to be in array recursion"); }; - array.push(value); - break JsonValue::Array(Arc::new(array)); + Arc::get_mut(&mut array).expect("sole writer to value").push(value); + break JsonValue::Array(array); }, RecursedValue::Object { partial, next_key } => loop { let value = match peek { @@ -343,17 +344,17 @@ fn take_value_recursive<'j, 's>( JsonValue::Str(create_cow(s)) } Peek::Array => { - let array = SmallVec::new(); + let array = Arc::new(SmallVec::new()); if let Some(next_peek) = parser.array_first()? { push_recursion!(next_peek, RecursedValue::Array(array)); // immediately jump to process the first value in the array continue 'recursion; } else { - JsonValue::Array(Arc::new(array)) + JsonValue::Array(array) } } Peek::Object => { - let object = LazyIndexMap::new(); + let object = Arc::new(LazyIndexMap::new()); if let Some(yet_another_key) = parser.object_first::(tape)? { push_recursion!( parser.peek()?, @@ -364,7 +365,7 @@ fn take_value_recursive<'j, 's>( ); continue 'recursion; } else { - JsonValue::Object(Arc::new(object)) + JsonValue::Object(object) } } _ => { @@ -387,7 +388,9 @@ fn take_value_recursive<'j, 's>( // now try to advance position in the current object if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { // object continuing - partial.insert(std::mem::replace(next_key, yet_another_key), value); + Arc::get_mut(partial) + .expect("sole writer") + .insert(std::mem::replace(next_key, yet_another_key), value); peek = parser.peek()?; continue; } @@ -396,8 +399,8 @@ fn take_value_recursive<'j, 's>( unreachable!("known to be in object recursion"); }; - partial.insert(next_key, value); - break JsonValue::Object(Arc::new(partial)); + Arc::get_mut(&mut partial).expect("sole writer").insert(next_key, value); + break JsonValue::Object(partial); }, }; @@ -413,27 +416,29 @@ fn take_value_recursive<'j, 's>( value = match current_recursion { RecursedValue::Array(array) => { if let Some(next_peek) = parser.array_step()? { - array.push(value); + Arc::get_mut(array).expect("sole writer").push(value); break next_peek; } let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { unreachable!("known to be in object recursion"); }; - array.push(value); - JsonValue::Array(Arc::new(array)) + Arc::get_mut(&mut array).expect("sole writer").push(value); + JsonValue::Array(array) } RecursedValue::Object { partial, next_key } => { if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { - partial.insert(std::mem::replace(next_key, yet_another_key), value); + Arc::get_mut(partial) + .expect("sole writer") + .insert(std::mem::replace(next_key, yet_another_key), value); break parser.peek()?; } let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { unreachable!("known to be in object recursion"); }; - partial.insert(next_key, value); - JsonValue::Object(Arc::new(partial)) + Arc::get_mut(&mut partial).expect("sole writer").insert(next_key, value); + JsonValue::Object(partial) } } }; From 50aa24d36991e2885edd35492d3e230cb8bca7b4 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 10 Sep 2024 10:08:36 +0100 Subject: [PATCH 5/6] move arc get_muts out of loop --- crates/jiter/src/value.rs | 276 +++++++++++++++++++------------------- 1 file changed, 140 insertions(+), 136 deletions(-) diff --git a/crates/jiter/src/value.rs b/crates/jiter/src/value.rs index bb5b00bf..2f360eeb 100644 --- a/crates/jiter/src/value.rs +++ b/crates/jiter/src/value.rs @@ -250,158 +250,162 @@ fn take_value_recursive<'j, 's>( 'recursion: loop { let mut value = match current_recursion { - RecursedValue::Array(array) => loop { - let value = match peek { - Peek::True => { - parser.consume_true()?; - JsonValue::Bool(true) - } - Peek::False => { - parser.consume_false()?; - JsonValue::Bool(false) - } - Peek::Null => { - parser.consume_null()?; - JsonValue::Null - } - Peek::String => { - let s = parser.consume_string::(tape, false)?; - JsonValue::Str(create_cow(s)) - } - Peek::Array => { - let array = Arc::new(SmallVec::new()); - if let Some(next_peek) = parser.array_first()? { - push_recursion!(next_peek, RecursedValue::Array(array)); - // immediately jump to process the first value in the array - continue 'recursion; - } else { - JsonValue::Array(array) + RecursedValue::Array(array) => { + let array = Arc::get_mut(array).expect("sole writer"); + loop { + let value = match peek { + Peek::True => { + parser.consume_true()?; + JsonValue::Bool(true) } - } - Peek::Object => { - let object = Arc::new(LazyIndexMap::new()); - if let Some(next_key) = parser.object_first::(tape)? { - push_recursion!( - parser.peek()?, - RecursedValue::Object { - partial: object, - next_key: create_cow(next_key) - } - ); - continue 'recursion; - } else { - JsonValue::Object(object) + Peek::False => { + parser.consume_false()?; + JsonValue::Bool(false) } - } - _ => { - let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); - match n { - Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), - Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), - Ok(NumberAny::Float(float)) => JsonValue::Float(float), - Err(e) => { - if !peek.is_num() { - return Err(json_error!(ExpectedSomeValue, parser.index)); - } else { - return Err(e); + Peek::Null => { + parser.consume_null()?; + JsonValue::Null + } + Peek::String => { + let s = parser.consume_string::(tape, false)?; + JsonValue::Str(create_cow(s)) + } + Peek::Array => { + let array = Arc::new(SmallVec::new()); + if let Some(next_peek) = parser.array_first()? { + push_recursion!(next_peek, RecursedValue::Array(array)); + // immediately jump to process the first value in the array + continue 'recursion; + } else { + JsonValue::Array(array) + } + } + Peek::Object => { + let object = Arc::new(LazyIndexMap::new()); + if let Some(next_key) = parser.object_first::(tape)? { + push_recursion!( + parser.peek()?, + RecursedValue::Object { + partial: object, + next_key: create_cow(next_key) + } + ); + continue 'recursion; + } else { + JsonValue::Object(object) + } + } + _ => { + let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); + match n { + Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), + Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), + Ok(NumberAny::Float(float)) => JsonValue::Float(float), + Err(e) => { + if !peek.is_num() { + return Err(json_error!(ExpectedSomeValue, parser.index)); + } else { + return Err(e); + } } } } + }; + + // now try to advance position in the current array + if let Some(next_peek) = parser.array_step()? { + array.push(value); + peek = next_peek; + // array continuing + continue; } - }; - // now try to advance position in the current array - if let Some(next_peek) = parser.array_step()? { - Arc::get_mut(array).expect("sole writer").push(value); - peek = next_peek; - // array continuing - continue; - } + let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { + unreachable!("known to be in array recursion"); + }; - let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { - unreachable!("known to be in array recursion"); - }; - - Arc::get_mut(&mut array).expect("sole writer to value").push(value); - break JsonValue::Array(array); - }, - RecursedValue::Object { partial, next_key } => loop { - let value = match peek { - Peek::True => { - parser.consume_true()?; - JsonValue::Bool(true) - } - Peek::False => { - parser.consume_false()?; - JsonValue::Bool(false) - } - Peek::Null => { - parser.consume_null()?; - JsonValue::Null - } - Peek::String => { - let s = parser.consume_string::(tape, false)?; - JsonValue::Str(create_cow(s)) - } - Peek::Array => { - let array = Arc::new(SmallVec::new()); - if let Some(next_peek) = parser.array_first()? { - push_recursion!(next_peek, RecursedValue::Array(array)); - // immediately jump to process the first value in the array - continue 'recursion; - } else { - JsonValue::Array(array) + Arc::get_mut(&mut array).expect("sole writer to value").push(value); + break JsonValue::Array(array); + } + } + RecursedValue::Object { partial, next_key } => { + let partial = Arc::get_mut(partial).expect("sole writer"); + loop { + let value = match peek { + Peek::True => { + parser.consume_true()?; + JsonValue::Bool(true) } - } - Peek::Object => { - let object = Arc::new(LazyIndexMap::new()); - if let Some(yet_another_key) = parser.object_first::(tape)? { - push_recursion!( - parser.peek()?, - RecursedValue::Object { - partial: object, - next_key: create_cow(yet_another_key) - } - ); - continue 'recursion; - } else { - JsonValue::Object(object) + Peek::False => { + parser.consume_false()?; + JsonValue::Bool(false) } - } - _ => { - let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); - match n { - Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), - Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), - Ok(NumberAny::Float(float)) => JsonValue::Float(float), - Err(e) => { - if !peek.is_num() { - return Err(json_error!(ExpectedSomeValue, parser.index)); - } else { - return Err(e); + Peek::Null => { + parser.consume_null()?; + JsonValue::Null + } + Peek::String => { + let s = parser.consume_string::(tape, false)?; + JsonValue::Str(create_cow(s)) + } + Peek::Array => { + let array = Arc::new(SmallVec::new()); + if let Some(next_peek) = parser.array_first()? { + push_recursion!(next_peek, RecursedValue::Array(array)); + // immediately jump to process the first value in the array + continue 'recursion; + } else { + JsonValue::Array(array) + } + } + Peek::Object => { + let object = Arc::new(LazyIndexMap::new()); + if let Some(yet_another_key) = parser.object_first::(tape)? { + push_recursion!( + parser.peek()?, + RecursedValue::Object { + partial: object, + next_key: create_cow(yet_another_key) + } + ); + continue 'recursion; + } else { + JsonValue::Object(object) + } + } + _ => { + let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); + match n { + Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), + Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), + Ok(NumberAny::Float(float)) => JsonValue::Float(float), + Err(e) => { + if !peek.is_num() { + return Err(json_error!(ExpectedSomeValue, parser.index)); + } else { + return Err(e); + } } } } + }; + + // now try to advance position in the current object + if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { + // object continuing + partial.insert(std::mem::replace(next_key, yet_another_key), value); + peek = parser.peek()?; + continue; } - }; - - // now try to advance position in the current object - if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { - // object continuing - Arc::get_mut(partial) - .expect("sole writer") - .insert(std::mem::replace(next_key, yet_another_key), value); - peek = parser.peek()?; - continue; - } - let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { - unreachable!("known to be in object recursion"); - }; + let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { + unreachable!("known to be in object recursion"); + }; - Arc::get_mut(&mut partial).expect("sole writer").insert(next_key, value); - break JsonValue::Object(partial); - }, + Arc::get_mut(&mut partial).expect("sole writer").insert(next_key, value); + break JsonValue::Object(partial); + } + } }; // current array or object has finished; From 546c4df0d3dfd8f11ffcb1d838934651f99b251c Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 10 Sep 2024 10:47:44 +0100 Subject: [PATCH 6/6] clippy, pull last value off stack --- crates/jiter/src/value.rs | 93 +++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 53 deletions(-) diff --git a/crates/jiter/src/value.rs b/crates/jiter/src/value.rs index 2f360eeb..78da22a6 100644 --- a/crates/jiter/src/value.rs +++ b/crates/jiter/src/value.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use std::sync::Arc; use num_bigint::BigInt; -use smallvec::{smallvec, SmallVec}; +use smallvec::SmallVec; use crate::errors::{json_error, JsonError, JsonResult, DEFAULT_RECURSION_LIMIT}; use crate::lazy_index_map::LazyIndexMap; @@ -223,9 +223,10 @@ enum RecursedValue<'s> { } #[inline(never)] // this is an iterative algo called only from take_value, no point in inlining +#[allow(clippy::too_many_lines)] // FIXME? fn take_value_recursive<'j, 's>( mut peek: Peek, - current_recursion: RecursedValue<'s>, + mut current_recursion: RecursedValue<'s>, parser: &mut Parser<'j>, tape: &mut Tape, recursion_limit: u8, @@ -234,22 +235,20 @@ fn take_value_recursive<'j, 's>( ) -> JsonResult> { let recursion_limit: usize = recursion_limit.into(); - let mut recursion_stack: SmallVec<[RecursedValue; 8]> = smallvec![current_recursion]; - let mut current_recursion = recursion_stack.last_mut().expect("just inserted one element"); + let mut recursion_stack: SmallVec<[RecursedValue; 8]> = SmallVec::new(); macro_rules! push_recursion { ($next_peek:expr, $value:expr) => { peek = $next_peek; + recursion_stack.push(std::mem::replace(&mut current_recursion, $value)); if recursion_stack.len() >= recursion_limit { return Err(json_error!(RecursionLimitExceeded, parser.index)); } - recursion_stack.push($value); - current_recursion = recursion_stack.last_mut().expect("just inserted one element"); }; } 'recursion: loop { - let mut value = match current_recursion { + let mut value = match &mut current_recursion { RecursedValue::Array(array) => { let array = Arc::get_mut(array).expect("sole writer"); loop { @@ -276,9 +275,8 @@ fn take_value_recursive<'j, 's>( push_recursion!(next_peek, RecursedValue::Array(array)); // immediately jump to process the first value in the array continue 'recursion; - } else { - JsonValue::Array(array) } + JsonValue::Array(array) } Peek::Object => { let object = Arc::new(LazyIndexMap::new()); @@ -291,23 +289,23 @@ fn take_value_recursive<'j, 's>( } ); continue 'recursion; - } else { - JsonValue::Object(object) } + JsonValue::Object(object) } _ => { - let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); - match n { - Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), - Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), - Ok(NumberAny::Float(float)) => JsonValue::Float(float), - Err(e) => { + let n = parser + .consume_number::(peek.into_inner(), allow_inf_nan) + .map_err(|e| { if !peek.is_num() { - return Err(json_error!(ExpectedSomeValue, parser.index)); + json_error!(ExpectedSomeValue, parser.index) } else { - return Err(e); + e } - } + })?; + match n { + NumberAny::Int(NumberInt::Int(int)) => JsonValue::Int(int), + NumberAny::Int(NumberInt::BigInt(big_int)) => JsonValue::BigInt(big_int), + NumberAny::Float(float) => JsonValue::Float(float), } } }; @@ -320,7 +318,7 @@ fn take_value_recursive<'j, 's>( continue; } - let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { + let RecursedValue::Array(mut array) = current_recursion else { unreachable!("known to be in array recursion"); }; @@ -354,9 +352,8 @@ fn take_value_recursive<'j, 's>( push_recursion!(next_peek, RecursedValue::Array(array)); // immediately jump to process the first value in the array continue 'recursion; - } else { - JsonValue::Array(array) } + JsonValue::Array(array) } Peek::Object => { let object = Arc::new(LazyIndexMap::new()); @@ -369,23 +366,23 @@ fn take_value_recursive<'j, 's>( } ); continue 'recursion; - } else { - JsonValue::Object(object) } + JsonValue::Object(object) } _ => { - let n = parser.consume_number::(peek.into_inner(), allow_inf_nan); - match n { - Ok(NumberAny::Int(NumberInt::Int(int))) => JsonValue::Int(int), - Ok(NumberAny::Int(NumberInt::BigInt(big_int))) => JsonValue::BigInt(big_int), - Ok(NumberAny::Float(float)) => JsonValue::Float(float), - Err(e) => { + let n = parser + .consume_number::(peek.into_inner(), allow_inf_nan) + .map_err(|e| { if !peek.is_num() { - return Err(json_error!(ExpectedSomeValue, parser.index)); + json_error!(ExpectedSomeValue, parser.index) } else { - return Err(e); + e } - } + })?; + match n { + NumberAny::Int(NumberInt::Int(int)) => JsonValue::Int(int), + NumberAny::Int(NumberInt::BigInt(big_int)) => JsonValue::BigInt(big_int), + NumberAny::Float(float) => JsonValue::Float(float), } } }; @@ -398,7 +395,7 @@ fn take_value_recursive<'j, 's>( continue; } - let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { + let RecursedValue::Object { mut partial, next_key } = current_recursion else { unreachable!("known to be in object recursion"); }; @@ -411,37 +408,27 @@ fn take_value_recursive<'j, 's>( // current array or object has finished; // try to pop and continue with the parent peek = loop { - if let Some(next_recursion) = recursion_stack.last_mut() { + if let Some(next_recursion) = recursion_stack.pop() { current_recursion = next_recursion; } else { return Ok(value); } value = match current_recursion { - RecursedValue::Array(array) => { + RecursedValue::Array(mut array) => { + Arc::get_mut(&mut array).expect("sole writer").push(value); if let Some(next_peek) = parser.array_step()? { - Arc::get_mut(array).expect("sole writer").push(value); + current_recursion = RecursedValue::Array(array); break next_peek; } - - let Some(RecursedValue::Array(mut array)) = recursion_stack.pop() else { - unreachable!("known to be in object recursion"); - }; - Arc::get_mut(&mut array).expect("sole writer").push(value); JsonValue::Array(array) } - RecursedValue::Object { partial, next_key } => { - if let Some(yet_another_key) = parser.object_step::(tape)?.map(create_cow) { - Arc::get_mut(partial) - .expect("sole writer") - .insert(std::mem::replace(next_key, yet_another_key), value); + RecursedValue::Object { mut partial, next_key } => { + Arc::get_mut(&mut partial).expect("sole writer").insert(next_key, value); + if let Some(next_key) = parser.object_step::(tape)?.map(create_cow) { + current_recursion = RecursedValue::Object { partial, next_key }; break parser.peek()?; } - - let Some(RecursedValue::Object { mut partial, next_key }) = recursion_stack.pop() else { - unreachable!("known to be in object recursion"); - }; - Arc::get_mut(&mut partial).expect("sole writer").insert(next_key, value); JsonValue::Object(partial) } }