🏗 Replace NoInline suffix check in runner.jar with the `@noinline…

…` closure annotation (ampproject#23363)

.eslintrc

@@ -81,6 +81,7 @@
         "export",
         "final",
         "nocollapse",
+        "noinline",
         "package",
         "record",
         "restricted",

build-system/eslint-rules/private-prop-names.js

@@ -38,14 +38,6 @@ module.exports = function(context) {
     return /_$/.test(fnName);
   }
 
-  /**
-   * @param {string}
-   * @return {boolean}
-   */
-  function hasExplicitNoInline(fnName) {
-    return /NoInline$/.test(fnName);
-  }
-
   /**
    * @param {!Node}
    * @return {boolean}
@@ -59,7 +51,6 @@ module.exports = function(context) {
     MethodDefinition: function(node) {
       if (
         hasPrivateAnnotation(node.leadingComments) &&
-        !hasExplicitNoInline(node.key.name) &&
         !hasTrailingUnderscore(node.key.name)
       ) {
         context.report({
@@ -73,7 +64,6 @@ module.exports = function(context) {
         node.parent.type == 'ExpressionStatement' &&
         hasPrivateAnnotation(node.parent.leadingComments) &&
         isThisMemberExpression(node.left) &&
-        !hasExplicitNoInline(node.left.property.name) &&
         !hasTrailingUnderscore(node.left.property.name)
       ) {
         context.report({

build-system/runner/src/org/ampproject/AmpCodingConvention.java

@@ -52,12 +52,6 @@ public AmpCodingConvention(CodingConvention convention) {
    * delivery), this could go away there.
    */
   @Override public boolean isExported(String name, boolean local) {
-    // This stops compiler from inlining functions (local or not) that end with
-    // NoInline in their name. Mostly used for externing try-catch to avoid v8
-    // de-optimization (https://goo.gl/gvzlDp)
-    if (name.endsWith("NoInline")) {
-      return true;
-    }
     // Bad hack, but we should really not try to inline CSS as these strings can
     // be very long.
     // See https://github.com/ampproject/amphtml/issues/10118

extensions/amp-analytics/0.1/amp-analytics.js

@@ -342,17 +342,17 @@ export class AmpAnalytics extends AMP.BaseElement {
               }
               trigger['selector'] = this.element.parentElement.tagName;
               trigger['selectionMethod'] = 'closest';
-              this.addTriggerNoInline_(trigger);
+              this.addTrigger_(trigger);
             } else if (trigger['selector']) {
               // Expand the selector using variable expansion.
               return this.variableService_
                 .expandTemplate(trigger['selector'], expansionOptions)
                 .then(selector => {
                   trigger['selector'] = selector;
-                  this.addTriggerNoInline_(trigger);
+                  this.addTrigger_(trigger);
                 });
             } else {
-              this.addTriggerNoInline_(trigger);
+              this.addTrigger_(trigger);
             }
           })
         );
@@ -374,13 +374,12 @@ export class AmpAnalytics extends AMP.BaseElement {
   }
 
   /**
-   * Calls `AnalyticsGroup.addTrigger` and reports any errors. "NoInline" is
-   * to avoid inlining this method so that `try/catch` does it veto
-   * optimizations.
+   * Calls `AnalyticsGroup.addTrigger` and reports any errors.
    * @param {!JsonObject} config
    * @private
+   * @noinline
    */
-  addTriggerNoInline_(config) {
+  addTrigger_(config) {
     if (!this.analyticsGroup_) {
       // No need to handle trigger for component that has already been detached
       // from DOM

extensions/amp-analytics/0.1/analytics-root.js

@@ -354,7 +354,7 @@ export class AnalyticsRoot {
         if (
           isSelectAny ||
           (isSelectRoot && target == rootElement) ||
-          matchesNoInline(target, selector)
+          tryMatches_(target, selector)
         ) {
           listener(target, event);
           // Don't fire the event multiple times even if the more than one
@@ -541,8 +541,9 @@ export class EmbedAnalyticsRoot extends AnalyticsRoot {
  * @param  {!Element} el
  * @param  {string} selector
  * @return {boolean}
+ * @noinline
  */
-function matchesNoInline(el, selector) {
+function tryMatches_(el, selector) {
   try {
     return matches(el, selector);
   } catch (e) {

extensions/amp-analytics/0.1/config.js

@@ -123,7 +123,7 @@ export class AnalyticsConfig {
     const configRewriterUrl = this.getConfigRewriter_()['url'];
 
     const config = dict({});
-    const inlineConfig = this.getInlineConfigNoInline();
+    const inlineConfig = this.getInlineConfig_();
     this.validateTransport_(inlineConfig);
     mergeObjects(inlineConfig, config);
     mergeObjects(this.remoteConfig_, config);
@@ -301,8 +301,9 @@ export class AnalyticsConfig {
   /**
    * @private
    * @return {!JsonObject}
+   * @noinline
    */
-  getInlineConfigNoInline() {
+  getInlineConfig_() {
     if (this.element_.CONFIG) {
       // If the analytics element is created by runtime, return cached config.
       return this.element_.CONFIG;

src/cookies.js

@@ -37,7 +37,7 @@ const TEST_COOKIE_NAME = '-test-amp-cookie-tmp';
  * @return {?string}
  */
 export function getCookie(win, name) {
-  const cookieString = tryGetDocumentCookieNoInline(win);
+  const cookieString = tryGetDocumentCookie_(win);
   if (!cookieString) {
     return null;
   }
@@ -58,12 +58,11 @@ export function getCookie(win, name) {
 
 /**
  * This method should not be inlined to prevent TryCatch deoptimization.
- * NoInline keyword at the end of function name also prevents Closure compiler
- * from inlining the function.
  * @param {!Window} win
  * @return {string}
+ * @noinline
  */
-function tryGetDocumentCookieNoInline(win) {
+function tryGetDocumentCookie_(win) {
   try {
     return win.document.cookie;
   } catch (e) {

src/service/custom-element-registry.js

@@ -70,7 +70,7 @@ export function upgradeOrRegisterElement(win, name, toClass) {
       element.tagName.toLowerCase() == name &&
       element.ownerDocument.defaultView == win
     ) {
-      tryUpgradeElementNoInline(element, toClass);
+      tryUpgradeElement_(element, toClass);
       // Remove element from array.
       stubbedElements.splice(i--, 1);
     }
@@ -79,13 +79,12 @@ export function upgradeOrRegisterElement(win, name, toClass) {
 
 /**
  * This method should not be inlined to prevent TryCatch deoptimization.
- * NoInline keyword at the end of function name also prevents Closure compiler
- * from inlining the function.
  * @param {Element} element
  * @param {function(new:../base-element.BaseElement, !Element)} toClass
  * @private
+ * @noinline
  */
-function tryUpgradeElementNoInline(element, toClass) {
+function tryUpgradeElement_(element, toClass) {
   try {
     element.upgrade(toClass);
   } catch (e) {

src/service/fixed-layer.js

@@ -214,7 +214,7 @@ export class FixedLayer {
    * @private
    */
   scanNode_(node, opt_lightboxMode) {
-    this.trySetupSelectorsNoInline(node, opt_lightboxMode);
+    this.trySetupSelectors_(node, opt_lightboxMode);
 
     // Sort tracked elements in document order.
     this.sortInDomOrder_();
@@ -565,13 +565,12 @@ export class FixedLayer {
    * Calls `setupSelectors_` in a try-catch.
    * Fails quietly with a dev error if call fails.
    * This method should not be inlined to prevent TryCatch deoptimization.
-   * NoInline keyword at the end of function name also prevents Closure compiler
-   * from inlining the function.
    * @param {!Node} root
    * @param {boolean=} opt_lightboxMode
    * @private
+   * @noinline
    */
-  trySetupSelectorsNoInline(root, opt_lightboxMode) {
+  trySetupSelectors_(root, opt_lightboxMode) {
     try {
       this.setupSelectors_(root, opt_lightboxMode);
     } catch (e) {

src/service/vsync-impl.js

@@ -406,15 +406,15 @@ export class Vsync {
     this.states_ = this.nextStates_;
     for (let i = 0; i < tasks.length; i++) {
       if (tasks[i].measure) {
-        if (!callTaskNoInline(tasks[i].measure, states[i])) {
+        if (!callTask_(tasks[i].measure, states[i])) {
           // Ensure that the mutate is not executed when measure fails.
           tasks[i].mutate = undefined;
         }
       }
     }
     for (let i = 0; i < tasks.length; i++) {
       if (tasks[i].mutate) {
-        callTaskNoInline(tasks[i].mutate, states[i]);
+        callTask_(tasks[i].mutate, states[i]);
       }
     }
     // Swap last arrays into double buffer.
@@ -452,8 +452,9 @@ export class Vsync {
  * For optimization reasons to stop try/catch from blocking optimization.
  * @param {function(!VsyncStateDef):undefined|undefined} callback
  * @param {!VsyncStateDef} state
+ * @noinline
  */
-function callTaskNoInline(callback, state) {
+function callTask_(callback, state) {
   devAssert(callback);
   try {
     const ret = callback(state);

src/style-installer.js

@@ -254,7 +254,7 @@ export function makeBodyVisible(doc) {
   const set = () => {
     bodyMadeVisible = true;
     setBodyVisibleStyles(doc);
-    renderStartedNoInline(doc);
+    renderStarted_(doc);
   };
 
   waitForBodyOpenPromise(doc)
@@ -306,8 +306,9 @@ function setBodyVisibleStyles(doc) {
 
 /**
  * @param {!Document} doc
+ * @noinline
  */
-function renderStartedNoInline(doc) {
+function renderStarted_(doc) {
   try {
     Services.resourcesForDoc(doc.documentElement).renderStarted();
   } catch (e) {