Limit recursion to 128 levels #163
Conversation
|
I tried a few ways of factoring this into a helper but they were all slower in benchmarks. 701d84e is ~3% slower. As is, this PR has no effect on benchmarks. |
|
Did you try slapping inline-always on the function? |
|
I tried #[cold]
#[inline(never)]
fn stack_overflow_error<T>(&mut self) -> Result<T> {
Err(self.peek_error(ErrorCode::Custom("recursion limit exceeded".into())))
}#[cold]
#[inline(never)]
fn stack_overflow_error(&mut self) -> Error {
self.peek_error(ErrorCode::Custom("recursion limit exceeded".into()))
}#[cold]
#[inline(never)]
fn stack_overflow_error() -> ErrorCode {
ErrorCode::Custom("recursion limit exceeded".into())
}Those range from 3% (the last one) to 10% (the middle one) slower than this PR. 10% is pretty astonishing given that we are talking about canada.json which is full of floating point numbers and parsing those is slow to begin with, so something weird is going on. I will check it out in |
|
I get pretty much the same slowdowns on my laptop Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz and my desktop Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz. |
| @@ -107,6 +108,7 @@ impl<R: Read> DeserializerImpl<R> { | |||
| DeserializerImpl { | |||
| read: read, | |||
| str_buf: Vec::with_capacity(128), | |||
| remaining_depth: 128, | |||
There was a problem hiding this comment.
How about adding a Deserializer::with_recursion_limit method so this can be customized? Or maybe a Deserializer builder with a with_recursion_limit method?
There was a problem hiding this comment.
I am keeping #162 open to decide whether this needs to be configurable.
| @@ -205,12 +207,30 @@ impl<R: Read> DeserializerImpl<R> { | |||
| visitor.visit_str(s) | |||
| } | |||
| b'[' => { | |||
| self.remaining_depth -= 1; | |||
There was a problem hiding this comment.
I suppose all this could be moved into the SeqVisitor but I'm not sure if that'd make things any more clean.
| @@ -523,6 +543,10 @@ impl<R: Read> DeserializerImpl<R> { | |||
| } | |||
| } | |||
|
|
|||
| fn stack_overflow() -> ErrorCode { | |||
| ErrorCode::Custom("recursion limit exceeded".into()) | |||
There was a problem hiding this comment.
It's a shame we can't add error codes in a backwards compatible fashion. Next major release though :)
|
I still can't make a better factored version run as fast. I will give it another shot once we have likely/unlikely rust-lang/rust#26179. |
No description provided.