I do not have enough samples to be sure, but you are right, I suspect that 255 might not be sufficient.

Is there any advantage to using a small integer? The value is stored right next to a Vec. The u8 will probably make the Deserializer bigger by a pointer size. So we can just use usize.

Fine by me.

Usize seems fine. But we will need to check benchmarks before merging because such a change would affect even users who do not change the recursion limit.

@dtolnay How do you check this? Do you have benchmarks at hand?

I would look at json-benchmark before and after the usize change.

cargo build \
    --release \
    --bin json-benchmark \
    --no-default-features \
    --features lib-serde,parse-dom,parse-struct,all-files

Benchmark results on my machine:

Before change:

                                DOM                STRUCT
======= serde_json ======= parse|stringify === parse|stringify ===
data/canada.json          13.2ms    12.7ms     4.8ms     7.7ms
data/citm_catalog.json     7.7ms     1.5ms     2.3ms     1.0ms
data/twitter.json          2.7ms     0.8ms     1.4ms     0.7ms

======= json-rust ======== parse|stringify === parse|stringify ===
data/canada.json           8.9ms     4.4ms
data/citm_catalog.json     4.6ms     1.0ms
data/twitter.json          1.6ms     0.6ms

==== rustc_serialize ===== parse|stringify === parse|stringify ===
data/canada.json          22.4ms    48.0ms    27.1ms    64.8ms
data/citm_catalog.json    16.9ms     4.1ms    21.4ms     2.9ms
data/twitter.json          7.7ms     1.7ms     9.8ms     1.5ms

Changing u8 to u32:

                                DOM                STRUCT
======= serde_json ======= parse|stringify === parse|stringify ===
data/canada.json          13.5ms    12.7ms     4.8ms     7.7ms
data/citm_catalog.json     7.7ms     1.5ms     2.3ms     1.0ms
data/twitter.json          2.7ms     0.8ms     1.4ms     0.7ms

======= json-rust ======== parse|stringify === parse|stringify ===
data/canada.json           8.9ms     4.4ms
data/citm_catalog.json     4.7ms     1.0ms
data/twitter.json          1.6ms     0.6ms

==== rustc_serialize ===== parse|stringify === parse|stringify ===
data/canada.json          22.7ms    47.8ms    26.7ms    65.2ms
data/citm_catalog.json    17.0ms     4.3ms    21.4ms     2.9ms
data/twitter.json          7.7ms     1.7ms     9.9ms     1.6ms

There's apparently a regression of 0.3ms on parsing canada.json.

Changing u8 to usize:

                                DOM                STRUCT
======= serde_json ======= parse|stringify === parse|stringify ===
data/canada.json          13.1ms    12.5ms     4.7ms     7.7ms
data/citm_catalog.json     7.6ms     1.5ms     2.3ms     1.0ms
data/twitter.json          2.7ms     0.8ms     1.3ms     0.7ms

======= json-rust ======== parse|stringify === parse|stringify ===
data/canada.json           9.0ms     4.5ms
data/citm_catalog.json     4.7ms     1.0ms
data/twitter.json          1.6ms     0.6ms

==== rustc_serialize ===== parse|stringify === parse|stringify ===
data/canada.json          21.9ms    47.6ms    26.9ms    64.9ms
data/citm_catalog.json    17.0ms     4.2ms    21.4ms     2.9ms
data/twitter.json          7.8ms     1.7ms     9.9ms     1.6ms

which looks like a 0.1ms improvement on canada.json and citm_catalog.json.

So, I'd go ahead with usize, if that's ok with you, @dtolnay.

Yes let's go with usize. Thanks for taking care of that!

@dtolnay Ready when you are :)

I think the remaining things to figure out here are:

• Naming convention for builder methods. In the standard library, typically with_ functions are constructors like Vec::with_capacity, and builder methods do not use a with_ prefix. Maybe fn recursion_limit would be more idiomatic.
• Self by value or &mut. In Strengthen the recommendation to use &mut self builders rust-lang/api-guidelines#81 we observed that self-by-value builders are almost never ideal. If this is one of the exceptions, how do we justify it? In your use case, would you always be invoking this on the same line as the Deserializer constructor?
• How to reconcile with an eventual serde_json::Format. I would feel more comfortable if we make more progress on Abstract over formats that deal with bytes serde#644 before merging this. That issue will involve some data structure that describes JSON as a format, including whether to use indentation when serializing and presumably the recursion limit for deserializing. It may turn out to be a better API to treat that data structure as a builder for serde_json::Deserializer, rather than having builder methods directly on Deserializer.

• Naming conventions: recursion_limit is fine by me.
• &mut: fine by me, too.
• fair enough, I'll open a PR that just increases the default limit and wait until there is consensus on Abstract over formats that deal with bytes serde#644

Could you check on the failing test case?

@dtolnay Ah, I hadn't seen that test. Fixed.

Of course, I realize that this since the default value of 1024 actually blows up Rust's stack, the patch may be problematic.

Yes, the whole point of the recursion limit is to limit the ability of untrusted JSON input to crash your program. 😿 Are you able to get by for now with a fork of serde_json that exposes this setting? I can try to get things moving on serde-rs/serde#644 again.

Let's continue to track this in #334.