Having a size limit probably does make sense. We do limit it for some script tags (e.g. amp-bind see the protoascii) and it can be done with max_bytes (part of Cdataspec).

Not sure what the right size is, but it might be worthwhile to follow amp-bind's lead with 100k. @choumx how did you come to decide on the 100k limit?

Somewhat arbitrarily. Since amp-state is just arbitrary data, the important part was to prevent publishers from throwing in the kitchen sink and unnecessarily bloating the page.

amp-experiment's JSON config is different in nature. A size constraint might make sense but it's worth thinking about what we're trying to prevent. There's already a timeout for the render-blocking mechanism right?

Thanks for putting this together, @torch2424. How did you arrive at the 3KB number? Curious to see what that translates to in real world.

Completely agree there should be reasonable limits, and like David's suggestion of enforcing this via AMP validation.

As for the 3KB number, let's see what a few examples look like... how many rules can be expressed within the limit etc to help inform the size restriction.

Unbounded size is not desirable, and would lead to performance issues.

@zhouyx / @honeybadgerdontcare

Having a size limit probably does make sense. We do limit it for some script tags (e.g. amp-bind see the protoascii) and it can be done with max_bytes (part of Cdataspec).

Good call on including @honeybadgerdontcare in this, didn't know the validator had support for this, glad that it does! 😄 🎉

@choumx

amp-experiment's JSON config is different in nature. A size constraint might make sense but it's worth thinking about what we're trying to prevent. There's already a timeout for the render-blocking mechanism right?

What we are trying to prevent is the amount of time it takes to read that JSON to not cut into their render blocking time. Our goal is 200ms. If we allow them to put in 20KB of javascript, that can deeply cut into their time. And we want to prevent that, as once we start mutating, we can't un-mutate.

For example (with made up numbers), if we allow the JSON to be large, and it takes 150ms to parse / eval. And then we take another 40ms verifying things, and then we start mutating, we can only apply a few mutations before we bail out, and not be able to un-mutate. Then it becomes, we can check that we may have the right amount of time to mutate. But if we take 150ms to parse/eval, just to decide nevermind, that's still not optimal.

Also, for our eventual server-side implementation, limiting this now would greatly help in scaling this in the future.

So I guess, TL;DR this is done for saving performance in the client side implementation (today), as well as implementing in the server (future). 😄

@jeffjose

Thanks for putting this together, @torch2424. How did you arrive at the 3KB number? Curious to see what that translates to in real world.

I got the number from a benchmark in a blog post I read a while ago when I originally wrote the design doc and outlined the constraint. I can write my own benchmark, but it would take a bit of time, which would definitely affect our timeline. 🤔

@kristoferbaxter

Completely agree there should be reasonable limits, and like David's suggestion of enforcing this via AMP validation. As for the 3KB number, let's see what a few examples look like... how many rules can be expressed within the limit etc to help inform the size restriction. Unbounded size is not desirable, and would lead to performance issues.

Definitely agree here, didn't implement in validator because didn't know 😄 Started playing with an example using this tool and realized 3KB may be a bit too small for what we are trying to express. So I'll probably increase this, to maybe like 10KB? And yes, unbounded size will lead into performance issues (in terms of UX and our deadline for mutating).

Thanks for all the feedback everyone! Because of this, I'm going to go ahead and close this for an implementation to the server. We can move there, or keep discussing here. Thank you! 😄

I did some quick tests on the size and 10KB appears to accommodate close to 80 simple mutations. (our limit is 70 mutations). So 10KB for a mixture of complex and simple mutations would work as a starting point.

10KB SGTM.