Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amp-bind: Expression complexity limit reached #11434

Closed
Gigliocom opened this issue Sep 26, 2017 · 61 comments
Closed

amp-bind: Expression complexity limit reached #11434

Gigliocom opened this issue Sep 26, 2017 · 61 comments

Comments

@Gigliocom
Copy link

What's the issue?

We need to perform over 50 expressions in unique AMP page, how can we do it?

How do we reproduce the issue?

Below a demo AMP product page with the errors.
https://www.giglio.com/borse-donna_borsa-mano-borsa-petite-2-jours-piccola-con-barra-metallica-logo-inciso-fendi-8bh2533wl.html?cSel=027&AAMMPP

What browsers are affected?

All browsers

Which AMP version is affected?

V0

@camelburrito
Copy link
Contributor

@Gigliocom - Thanks for reaching out! Could you provide detailed steps on how to reproduce the issue (and what this issue is).

I did not see any errors on the link above.

@Gigliocom
Copy link
Author

Hi,
when you click on thumb to choose another color, the main photo must be change and the hidden values into form must change also. But that does not.

In console.log appear this:
error.js:169 amp-bind: Expression eval failed with error: Expression size (52) exceeds max (50). Please reduce number of operands.​​​
Uncaught (in promise) Error: amp-bind: Expression eval failed with error: Expression size (52) exceeds max (50). Please reduce number of operands.​​​ reported

@jridgewell
Copy link
Contributor

/to @choumx

@dreamofabear
Copy link

dreamofabear commented Oct 3, 2017

Hi Giglio.com, thanks for your feedback. The expression complexity limit is intended to keep pages using amp-bind fast and responsive.

One way you can simplify your page expressions (and speed up the page itself!) is to reduce the number of variables. Specifically, I think you can remove the selectedSlideFor* variables.

Instead of showing/hiding 4 different <amp-carousel> elements based on the selected color, try using a single <amp-carousel> with its <amp-img> children changing their src via a [src] binding. For example:

<amp-state id="product">
  {
    "027": {
      "image1": "images/027/1.jpg",
      "image2": "images/027/2.jpg",
      ...
    }
  }
</amp-state>

<amp-carousel>
  <amp-img [src]="product[selectedColor].image1"></amp-img> 
  <amp-img [src]="product[selectedColor].image2"></amp-img> 
</amp-carousel>

Hope that helps!

@Gigliocom
Copy link
Author

Hi @choumx,
thanks for your help, but i think that this is not solution. We have products that has many photo per colors but unfortunately any colors dont'have the same number of pictures.

There is an another way to execute more than 50 expressions?

@dreamofabear
Copy link

We have products that has many photo per colors but unfortunately any colors dont'have the same number of pictures.

Ah, yes. We're planning to support more dynamic slide content in amp-carousel (#8921). One temporary workaround is to create a carousel with the max number of images among your products, and hide or replace overflow images with a stock image.

There is an another way to execute more than 50 expressions?

Not at the moment, though we may revisit this if it's a common problem -- I'll use this issue to track. Intermediary variables would also help reduce duplication of expression fragments in your case (#8397).

Sorry that you're have difficulties with amp-bind. We'll try to improve this soon.

@dreamofabear dreamofabear changed the title Expression eval failed with error amp-bind: Expression complexity limit reached Oct 4, 2017
@ampprojectbot
Copy link
Member

This issue doesn't have a category which makes it harder for us to keep track of it. @choumx Please add an appropriate category.

@ivanliu8
Copy link

ivanliu8 commented Nov 6, 2017

Definitely need to increase the limit. I'm creating a page with personalised products. Customer have options to choose designs, colours, pictures, fonts and enter custom text. We are using amp-bind to build up a preview link. 50 operands are definitely not enough. Alternatively, is there any ways I can encode the state json object and pass to the server as an parameter?

@dreamofabear
Copy link

@ivanliu8 Can you share a sample page with expressions that exceed this limit?

@leonahliang90
Copy link
Contributor

may i know how do you calculate the operands ? such as AstNodeType.NOT is equivalent to how many operands , AstNodeType.TERNARY is equivalent to how many operands, a reference to the source code would be much appreciated. Thx in advanced ! @choumx

@dreamofabear
Copy link

"Operand" is loosely defined here. 😄 It's technically the number of nodes in the abstract syntax tree of the expression.

I'd really appreciate any example pages that exceed the operand limit. This way we can thoughtfully support your use cases without necessarily changing the performance characteristics of amp-bind.

@leonahliang90
Copy link
Contributor

a work around trick would be:

based on the documentation, input would support both tap and change actions. So, basically you can create a transparent dummy input in front of your <element>

a simple demo as below:

  1. set the parent position: relative

code:

<input type="checkbox" class="dummy-checkbox" on="
tap: AMP.setState({
    50  operands bla bla bla
});
change: AMP.setState({
    50 operands bla bla bla 
})">

css:
.dummy-checkbox {
    opacity: 0;
    position: absolute;
    width: 100%;
    height: 100%;
    z-index: 1;
}

@gtonysam
Copy link

gtonysam commented Feb 6, 2018

HI ,

I am facing the same issue, I try to pass say 10 or more state parameters in url, but it is failing
amp-bind: Expression compile error in Expression size (61) exceeds max (50). Please reduce number of operands.

@dreamofabear
Copy link

dreamofabear commented Feb 6, 2018

@gtonysam Which component URL are you mutating? amp-list or amp-state?

Something like #13079 may help you by allowing use of state variables in URL substitutions without a complex expression.

@gtonysam
Copy link

gtonysam commented Feb 7, 2018

@choumx Hi we are using amp-state to bind the values to the url.
We have as many as 20 paramters to pass to the url. We tried amp-bind macro as well to add values but still the below issue comes always- amp-bind :Expression compile error in Expression size () exceeds max (50). Please reduce number of operands.

@dreamofabear
Copy link

Can you share the markup for this element?

@gtonysam
Copy link

gtonysam commented Feb 8, 2018

Hi Choumx,

This is what I am trying to do,
<amp-bind-macro id="dynamicDate" arguments="" expression="searchForm.departureDay+searchForm.departureMonthURL+searchForm.departureMonth+searchForm.departureYearURL" />

<amp-bind-macro id="LocationURL" arguments="" expression="searchForm.originLocationURL+searchForm.source+searchForm.destinationLocationURL+searchForm.destination" />

<amp-bind-macro id="dynamicURL" arguments="" expression="searchForm.baseurl+searchForm.tripType+dynamicDate()+LocationURL()+searchForm.endURL" />
This is just a part of url and few more operands needs to be added and I am unable to add one more operand as it exceed the limit

@dreamofabear
Copy link

Where is dynamicURL used in your case?

It looks like most of these variables are from <input> fields of a form. Have you considered flipping the logic? E.g. use amp-bind to modify the values of these inputs (e.g. [value] binding) and submit the form when ready?

@craigscott29
Copy link

craigscott29 commented May 9, 2018

I'm running up against this issue as well. Currently sitting at 6 over the 50 limit. I'm building a filterable inventory list that gets its JSON data from our ERP systems API. I'm trying to find ways to reduce the operands in my on change event but not seeing it yet - still working on it though. My second amp-list (Machine Make) is the one that is maxing out on me.

I'm trying to build the list so that it filters on each drop down selection, currently I have it working with the first two but when I add the model filter in that determines what to populate the third dropdown with it maxes out on me.

            <amp-list height="70" layout="fixed-height" src="<?php get_template_directory() ?>/json/inv-dropdowns.json">
                <template type="amp-mustache">
                    <!--<select id="machine-type" class="inv-dropdown" on="change:AMP.setState({inventoryMachineType: inventory.items.filter(a => event.value == 'all' ? true : a.Type == event.value),oem: dropdown.items[0].type.filter(x => x.name == event.value)[0]})">-->
                    <label for="type">Machine Type</label>
                    <select id="type" name="typeFilter" class="inv-dropdown" on="change:AMP.setState({inventory: {searchType: event.value,listSrc: '<?php get_template_directory() ?>/json/'+event.value+'.json'},oem: dropdown.items[0].type.filter(x => x.FilterName == event.value)[0]})">
                         <option value="all">Select your machine type</option>
                        {{#type}}
                        <option value="{{FilterName}}">{{name}}</option>
                        {{/type}}
                    </select>
                </template>
            </amp-list>
            
            <amp-list height="70" layout="fixed-height" [src]="oem" src="<?php get_template_directory() ?>/json/inv-dropdowns.json">
                <template type="amp-mustache">
                    <label for="oem">Machine Make</label>
                    <select id="oem" name="oemFilter" [disabled]="!oem" disabled class="inv-dropdown" on="change:AMP.setState({inventory: {listSrc: {{FilterName}}.items.filter(b => event.value == 'all' ? true : b.Part_CommercialBrand == event.value)},model: dropdown.items[0].type[{{id}}].oem.filter(y => y.make == event.value)[0]})">
                        <option value="all">Select your machine make</option>
                        {{#oem}}
                        <option value="{{make}}">{{make}}</option>
                        {{/oem}}
                    </select>
                </template>                
            </amp-list>
            
            <amp-list height="70" layout="fixed-height" [src]="model" src="<?php get_template_directory() ?>/json/inv-dropdowns.json">
                <template type="amp-mustache">
                    <label for="model">Machine Make</label>
                    <select id="model" [disabled]="!model" disabled class="inv-dropdown">
                        {{^model}}
                        <option value="">Select your machine model</option>{{/model}} {{#model.0}}
                        <option value="">Select your machine model</option>{{/model.0}} {{#model}}
                        <option value="{{.}}">{{.}}</option>{{/model}}
                    </select>
                </template>                
            </amp-list>
            
            <amp-state id="dropdown" src="<?php get_template_directory() ?>/json/inv-dropdowns.json"></amp-state>

@dreamofabear
Copy link

On a side note, it would be very useful to be able to “stringify” an amp-state object or a portion of it, because I frequently end up in situations where I'd like to pass the state (or parts of it) in a server request. (Attempts to do this with expressions is a good example of how you hit the complexity limit, by the way). It definitely also be useful for aforementioned iframe communication.

@Makac Thanks for the feature request. We could do this by adding a new URL replacement variable that reads from amp-state data, e.g. <amp-list src="https://data.com?foo=STATE(foo)&foobar=STATE(foo.bar)">. Though perhaps an easier way is to stop counting dereferences as an operand.

Currently sitting at 6 over the 50 limit. I'm building a filterable inventory list that gets its JSON data from our ERP systems API. I'm trying to find ways to reduce the operands in my on change event but not seeing it yet - still working on it though. My second amp-list (Machine Make) is the one that is maxing out on me.

@craigscott29 Thanks for posting your use case. I think it looks reasonable.

Since people are hitting the "50 operand" limit much more often than the "2000 binding" limit, I'm going to see if we can shift this balance to something like 100 operands and 1000 bindings and still retain (roughly) the same performance bounds.

Thanks for your patience everyone.

@craigscott29
Copy link

@choumx any chance of the operand increase having a quick turn around? The president of the company I work for wants the inventory list live in the next 2 weeks haha.

@dreamofabear
Copy link

@craigscott29 Given our weekly release cycle, unlikely. :(

In the meantime, you can try performing some of the filtering server-side. I.e. change the second amp-list's endpoint via a [src] binding that contains the parameters from the first.

@craigscott29
Copy link

craigscott29 commented May 9, 2018

@choumx, thanks. I shall take a look into it. I had tried something like that in my first attempt by following this example but I couldn't seem to get the filtering to work. Variables were passing correctly to the query string, it just wasn't doing anything.

@dreamofabear
Copy link

It won't do anything automatically. That sample shows server-side filtering/sorting -- the idea is to read the query parameters on the server and then perform the sorting on the server.

OTOH, your current code performs filtering/sorting on the client. This is fine for simple examples but can hit complexity limits as you've seen. :) Generally, AMP strives to "solve problems at the right layer", so we believe that more complex business logic should be done on the server, not the client.

Of course there are shades of gray in between -- sometimes it's nice to not perform a network request when changing a filter. For your case, since you're performing an network request anyways, doing the filtering on the server should work. Hope that makes sense!

@craigscott29
Copy link

I may be thinking about this incorrectly, but I think the server-side is where my issue may be. The data I'm retrieving doesn't exist on the same server as the site. I'm scraping it from our ERP systems API which is data output only. Essentially, I can retrieve my block of data in one pull (which is all I'd want to do because it's a slow system) and can't query against the server itself.

I'll maybe try setting up a service to pull the data down daily to the web server.

@dreamofabear
Copy link

Ah, I see. Yea, having a flexible server is very useful when developing with AMP in general.

@zoezl
Copy link

zoezl commented May 15, 2018

Hi @choumx

I'm trying to render an expression with amp-list and facing the same issue.

This is for calculating the price of merchandise with different additions.
The number of elements in attr array and content array is uncertain. I have no idea how this could work within the limit.

<amp-list src="json/prodetails.json" layout="fill" id="list_id">
    <template type="amp-mustache">
      <p [text]="
        {{#sliver}}
        {{#attr}}
          materialData.items[0].slive[0].attr[{{id}}].content[pro.{{name}}Id?(+pro.{{name}}Id)+1:0].price
        {{/attr}}
        {{/sliver}}
      "></p>
    </template>
  </amp-list>

Is there any workaround?

@leonahliang90
Copy link
Contributor

leonahliang90 commented May 16, 2018

Since people are hitting the "50 operand" limit much more often than the "2000 binding" limit, I'm going to see if we can shift this balance to something like 100 operands and 1000 bindings and still retain (roughly) the same performance bounds.

To be frank, we are not really sure how to do the counting for the bindings, currently we got 2 concerns about reducing the "2000" bindings limit down to "1000" :

  1. We do not know how the "garbage collector disposal" logic works, but as long as it works like a charm, I think very rare case it will hit the bindings limit. We did encounter 2000 bindings limit issue before, please refer to amp-list: [src] changes don't properly dispose old bindings  #12517, and based on my wild guess, the root cause was <amp-state [src]>, it seems like the garbage collector disposal logics did not work well with <amp-state [src]>.
  2. There are use cases that new items are being appended to <amp-list> upon user clicks on "View More" button and what the action does is to submit a <form> and keep accumulating the data [src] of the list. Our concern is will this action cause the bindings limit issue if users click on "View More" button a lot of times ?

While for the decision of increasing the operands from 50 to 100, we are quite happy with it, but it may lead to another question. FYI, due to the "less logic restriction" of mustache template, we are writing most of our if...else... logic with <div [class]="true ? 'show' : 'hide'" approach, and if we expand the operands from 50 to 100, I can foresee there will be some blinkings performance issue as some developers might write some very complex hide & show condition on the <div [class]> elements. (upon every AMP.setState which will cause the entire [property] to be re-rendered.)

@dreamofabear
Copy link

@leonalicious Thanks for the feedback.

We do not know how the "garbage collector disposal" logic works, but as long as it works like a charm, I think very rare case it will hit the bindings limit.

Sorry about that bug again.

There are use cases that new items are being appended to upon user clicks on "View More" button and what the action does is to submit a

and keep accumulating the data [src] of the list.

This is a good point and may come up more often once amp-list's infinite scroll feature is released. I think it's reasonable to allow the binding limit to increase for those use cases. Eventually we may need to implement a more robust performance solution.

@dreamofabear
Copy link

dreamofabear commented May 23, 2018

@gtonysam @Makac We recently added the AMP_STATE(...) URL variable so you can use that now instead of long amp-bind expressions (#15427).

See the URL variable substitutions docs for more detail.

@dreamofabear
Copy link

Update: In the last 7 days, there have been < 50 instances where an AMP page exceeds 1000 bindings (including runtime changes e.g. "view more" button) across very few domains.

I think we can safely rebalance the complexity limit to 100 operands and 1000 bindings, which should help unblock many use cases. I'll open another issue to address the "view more" and infinite scroll use case.

@dreamofabear
Copy link

Please see #15519 for discussion on binding limits vis-a-vis infinite scroll/view more.

@dreamofabear
Copy link

Fixed by #15517.

@JakobEichler
Copy link

I'd like to execute this assignment, but I get this error:

amp-bind: Expression eval failed. Expression size (104) exceeds max (100). Please reduce number of operands.​​​
Error: amp-bind: Expression eval failed. Expression size (104) exceeds max (100). Please reduce number of operands.​​​

I made my response so nested, because I wanted support for global variables in an amp-list template where I also use the same endpoint.

@dreamofabear
Copy link

Can you post an example of this large expression please?

@JakobEichler
Copy link

<form id="show-more-form-default-search" method="POST" action-xhr="<?php echo $srcSearch?>" on="submit-success: AMP.setState( { searchMeta: { offset: searchMeta.offset + <?php echo $limitShowMore?> }, defaultParkingPlaces: { my_response: [ { items: defaultParkingPlaces.my_response[0].items.concat( event.response.items), resultsCount: event.response.my_response[0].resultsCount, has_results: event.response.my_response[0].has_results, has_more_results: event.response.my_response[0].has_more_results, timeSpanStr: event.response.my_response[0].timeSpanStr, placeName: event.response.my_response[0].placeName } ], } } );">

But I changed my response and removed the my_response[0]. part and it works now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests