Skip to content

Commit

Permalink
Rebalance amp-bind limits.
Browse files Browse the repository at this point in the history
  • Loading branch information
William Chou committed May 23, 2018
1 parent b62a235 commit 61f7c44
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 12 deletions.
7 changes: 3 additions & 4 deletions extensions/amp-bind/0.1/bind-expression.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,10 @@ const TAG = 'amp-bind';
export let BindExpressionResultDef;

/**
* Default maximum number of nodes in an expression AST.
* Double size of a "typical" expression in examples/bind/performance.amp.html.
* Maximum number of nodes in an expression AST.
* @const @private {number}
*/
const DEFAULT_MAX_AST_SIZE = 50;
const MAX_AST_SIZE = 100;

/** @const @private {string} */
const BUILT_IN_FUNCTIONS = 'built-in-functions';
Expand Down Expand Up @@ -210,7 +209,7 @@ export class BindExpression {
this.expressionSize = this.numberOfNodesInAst_(this.ast_);

// Check if this expression string is too large (for performance).
const maxSize = opt_maxAstSize || DEFAULT_MAX_AST_SIZE;
const maxSize = opt_maxAstSize || MAX_AST_SIZE;
const skipConstraint = getMode().localDev && !getMode().test;
if (this.expressionSize > maxSize && !skipConstraint) {
throw new Error(`Expression size (${this.expressionSize}) exceeds max ` +
Expand Down
10 changes: 2 additions & 8 deletions extensions/amp-bind/0.1/bind-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class Bind {
* Upper limit on number of bindings for performance.
* @private {number}
*/
this.maxNumberOfBindings_ = 2000; // Based on ~1ms to parse an expression.
this.maxNumberOfBindings_ = 1000; // Based on ~2ms to parse an expression.

/** @const @private {!../../../src/service/resources-impl.Resources} */
this.resources_ = Services.resourcesForDoc(ampdoc);
Expand Down Expand Up @@ -502,12 +502,6 @@ export class Bind {
: this.maxNumberOfBindings_ - this.numberOfBindings();

return this.scanNode_(node, limit).then(results => {
// Measuring impact of possibly reducing the binding limit (#11434).
const numberOfBindings = this.numberOfBindings();
if (numberOfBindings > 1000) {
dev().expectedError(TAG, `Over 1000 bindings (${numberOfBindings}).`);
}

const {
boundElements, bindings, expressionToElements, limitExceeded,
} = results;
Expand All @@ -516,7 +510,7 @@ export class Bind {
Object.assign(this.expressionToElements_, expressionToElements);

if (limitExceeded) {
user().error(TAG, 'Maximum number of bindings reached ' +
dev().expectedError(TAG, 'Maximum number of bindings reached ' +
`(${this.maxNumberOfBindings_}). Additional elements with ` +
'bindings will be ignored.');
}
Expand Down

0 comments on commit 61f7c44

Please sign in to comment.