Disable syntax computation on very long diffs
Computing the syntax highlighting for diffs of very long files can be slow -- even when the delta is small and the context is not set to "whole file". This is partly because, by the nature of parsing, the computation must start at the beginning of the file, even if the visible delta is in the middle or at the end. Currently, the computation also continues to the end of the file, regardless of whether that content is visible. This slowness can be problematic when rendering diffs inline, because inline diffs are rendered in order, and the n'th diff will not begin rendering until the n-1'th diff has finished computing syntax highlighting. This cause can be hidden from users, because the work that the browser is doing is often never displayed. This situation is similar to the safety-bypass mechanism that helps users avoid very slow renders on large files when they have enabled "whole file" context. The diff builder already houses logic to avoid computing syntax highlighting when the diff contains any very long line. This was added to avoid needless highlighting of machine-generated / minified files. With this change, the diff builder takes the diff length into account, in addition to individual line length. The notion of the overall diff length is borrowed from the safety bypass. Because a diff represents two files, it can describe two different file lengths. The length computation used in the safety bypass approximates the number of vertical lines that would appear in a side-by-side diff, and it's appropriate here because it's again being used to approximate the computational effort of processing a diff. Change-Id: I1d24846e550eb631bc791267630ae1ed511b6f75
This commit is contained in:
		
				
					committed by
					
						
						Kasper Nilsson
					
				
			
			
				
	
			
			
			
						parent
						
							d3a779a8b4
						
					
				
				
					commit
					7e9b0c382f
				
			@@ -76,6 +76,9 @@ limitations under the License.
 | 
			
		||||
      // syntax highlighting for the entire file.
 | 
			
		||||
      const SYNTAX_MAX_LINE_LENGTH = 500;
 | 
			
		||||
 | 
			
		||||
      // Disable syntax highlighting if the overall diff is too large.
 | 
			
		||||
      const SYNTAX_MAX_DIFF_LENGTH = 20000;
 | 
			
		||||
 | 
			
		||||
      const TRAILING_WHITESPACE_PATTERN = /\s+$/;
 | 
			
		||||
 | 
			
		||||
      Polymer({
 | 
			
		||||
@@ -190,7 +193,7 @@ limitations under the License.
 | 
			
		||||
                this.dispatchEvent(new CustomEvent('render-content',
 | 
			
		||||
                    {bubbles: true}));
 | 
			
		||||
 | 
			
		||||
                if (this._anyLineTooLong()) {
 | 
			
		||||
                if (this._diffTooLargeForSyntax()) {
 | 
			
		||||
                  this.$.syntaxLayer.enabled = false;
 | 
			
		||||
                }
 | 
			
		||||
 | 
			
		||||
@@ -473,10 +476,32 @@ limitations under the License.
 | 
			
		||||
          }, false);
 | 
			
		||||
        },
 | 
			
		||||
 | 
			
		||||
        _diffTooLargeForSyntax() {
 | 
			
		||||
          return this._anyLineTooLong() ||
 | 
			
		||||
              this.getDiffLength() > SYNTAX_MAX_DIFF_LENGTH;
 | 
			
		||||
        },
 | 
			
		||||
 | 
			
		||||
        setBlame(blame) {
 | 
			
		||||
          if (!this._builder || !blame) { return; }
 | 
			
		||||
          this._builder.setBlame(blame);
 | 
			
		||||
        },
 | 
			
		||||
 | 
			
		||||
        /**
 | 
			
		||||
         * Get the approximate length of the diff as the sum of the maximum
 | 
			
		||||
         * length of the chunks.
 | 
			
		||||
         * @return {number}
 | 
			
		||||
         */
 | 
			
		||||
        getDiffLength() {
 | 
			
		||||
          return this.diff.content.reduce((sum, sec) => {
 | 
			
		||||
            if (sec.hasOwnProperty('ab')) {
 | 
			
		||||
              return sum + sec.ab.length;
 | 
			
		||||
            } else {
 | 
			
		||||
              return sum + Math.max(
 | 
			
		||||
                  sec.hasOwnProperty('a') ? sec.a.length : 0,
 | 
			
		||||
                  sec.hasOwnProperty('b') ? sec.b.length : 0);
 | 
			
		||||
            }
 | 
			
		||||
          }, 0);
 | 
			
		||||
        },
 | 
			
		||||
      });
 | 
			
		||||
    })();
 | 
			
		||||
  </script>
 | 
			
		||||
 
 | 
			
		||||
@@ -1052,6 +1052,10 @@ limitations under the License.
 | 
			
		||||
        });
 | 
			
		||||
      });
 | 
			
		||||
 | 
			
		||||
      test('getDiffLength', () => {
 | 
			
		||||
        assert.equal(element.getDiffLength(diff), 52);
 | 
			
		||||
      });
 | 
			
		||||
 | 
			
		||||
      test('getContentByLine', () => {
 | 
			
		||||
        let actual;
 | 
			
		||||
 | 
			
		||||
 
 | 
			
		||||
@@ -310,7 +310,7 @@ limitations under the License.
 | 
			
		||||
    <div id="sizeWarning" class$="[[_computeWarningClass(_showWarning)]]">
 | 
			
		||||
      <p>
 | 
			
		||||
        Prevented render because "Whole file" is enabled and this diff is very
 | 
			
		||||
        large (about [[_diffLength(diff)]] lines).
 | 
			
		||||
        large (about [[_diffLength]] lines).
 | 
			
		||||
      </p>
 | 
			
		||||
      <gr-button on-tap="_handleLimitedBypass">
 | 
			
		||||
        Render with limited context
 | 
			
		||||
 
 | 
			
		||||
@@ -195,6 +195,8 @@
 | 
			
		||||
          return this._createCommentThreadGroup.bind(this);
 | 
			
		||||
        },
 | 
			
		||||
      },
 | 
			
		||||
 | 
			
		||||
      _diffLength: Number,
 | 
			
		||||
    },
 | 
			
		||||
 | 
			
		||||
    behaviors: [
 | 
			
		||||
@@ -624,6 +626,7 @@
 | 
			
		||||
 | 
			
		||||
    _diffChanged(newValue) {
 | 
			
		||||
      if (newValue) {
 | 
			
		||||
        this._diffLength = this.$.diffBuilder.getDiffLength();
 | 
			
		||||
        this._renderDiffTable();
 | 
			
		||||
      }
 | 
			
		||||
    },
 | 
			
		||||
@@ -634,7 +637,7 @@
 | 
			
		||||
        return;
 | 
			
		||||
      }
 | 
			
		||||
      if (this.prefs.context === -1 &&
 | 
			
		||||
          this._diffLength(this.diff) >= LARGE_DIFF_THRESHOLD_LINES &&
 | 
			
		||||
          this._diffLength >= LARGE_DIFF_THRESHOLD_LINES &&
 | 
			
		||||
          this._safetyBypass === null) {
 | 
			
		||||
        this._showWarning = true;
 | 
			
		||||
        this.dispatchEvent(new CustomEvent('render', {bubbles: true}));
 | 
			
		||||
@@ -684,25 +687,6 @@
 | 
			
		||||
      return items.length === 0;
 | 
			
		||||
    },
 | 
			
		||||
 | 
			
		||||
    /**
 | 
			
		||||
     * The number of lines in the diff. For delta chunks that are different
 | 
			
		||||
     * sizes on the left and the right, the longer side is used.
 | 
			
		||||
     * @param {!Object} diff
 | 
			
		||||
     * @return {number}
 | 
			
		||||
     */
 | 
			
		||||
    _diffLength(diff) {
 | 
			
		||||
      return diff.content.reduce((sum, sec) => {
 | 
			
		||||
        if (sec.hasOwnProperty('ab')) {
 | 
			
		||||
          return sum + sec.ab.length;
 | 
			
		||||
        } else {
 | 
			
		||||
          return sum + Math.max(
 | 
			
		||||
              sec.hasOwnProperty('a') ? sec.a.length : 0,
 | 
			
		||||
              sec.hasOwnProperty('b') ? sec.b.length : 0
 | 
			
		||||
          );
 | 
			
		||||
        }
 | 
			
		||||
      }, 0);
 | 
			
		||||
    },
 | 
			
		||||
 | 
			
		||||
    _handleFullBypass() {
 | 
			
		||||
      this._safetyBypass = FULL_CONTEXT;
 | 
			
		||||
      this._renderDiffTable();
 | 
			
		||||
 
 | 
			
		||||
@@ -55,12 +55,6 @@ limitations under the License.
 | 
			
		||||
      assert.isTrue(cancelStub.calledOnce);
 | 
			
		||||
    });
 | 
			
		||||
 | 
			
		||||
    test('_diffLength', () => {
 | 
			
		||||
      element = fixture('basic');
 | 
			
		||||
      const mock = document.createElement('mock-diff-response');
 | 
			
		||||
      assert.equal(element._diffLength(mock.diffResponse), 52);
 | 
			
		||||
    });
 | 
			
		||||
 | 
			
		||||
    test('line limit with line_wrapping', () => {
 | 
			
		||||
      element = fixture('basic');
 | 
			
		||||
      element.prefs = {line_wrapping: true, line_length: 80, tab_size: 2};
 | 
			
		||||
@@ -971,6 +965,7 @@ limitations under the License.
 | 
			
		||||
                  new CustomEvent('render', {bubbles: true}));
 | 
			
		||||
            });
 | 
			
		||||
        const mock = document.createElement('mock-diff-response');
 | 
			
		||||
        sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
 | 
			
		||||
        element.diff = mock.diffResponse;
 | 
			
		||||
        element.comments = {left: [], right: []};
 | 
			
		||||
        element.noRenderOnPrefsChange = true;
 | 
			
		||||
@@ -978,7 +973,6 @@ limitations under the License.
 | 
			
		||||
 | 
			
		||||
      test('large render w/ context = 10', done => {
 | 
			
		||||
        element.prefs = {context: 10};
 | 
			
		||||
        sandbox.stub(element, '_diffLength', () => 10000);
 | 
			
		||||
        element.addEventListener('render', () => {
 | 
			
		||||
          assert.isTrue(renderStub.called);
 | 
			
		||||
          assert.isFalse(element._showWarning);
 | 
			
		||||
@@ -990,7 +984,6 @@ limitations under the License.
 | 
			
		||||
      test('large render w/ whole file and bypass', done => {
 | 
			
		||||
        element.prefs = {context: -1};
 | 
			
		||||
        element._safetyBypass = 10;
 | 
			
		||||
        sandbox.stub(element, '_diffLength', () => 10000);
 | 
			
		||||
        element.addEventListener('render', () => {
 | 
			
		||||
          assert.isTrue(renderStub.called);
 | 
			
		||||
          assert.isFalse(element._showWarning);
 | 
			
		||||
@@ -1001,7 +994,6 @@ limitations under the License.
 | 
			
		||||
 | 
			
		||||
      test('large render w/ whole file and no bypass', done => {
 | 
			
		||||
        element.prefs = {context: -1};
 | 
			
		||||
        sandbox.stub(element, '_diffLength', () => 10000);
 | 
			
		||||
        element.addEventListener('render', () => {
 | 
			
		||||
          assert.isFalse(renderStub.called);
 | 
			
		||||
          assert.isTrue(element._showWarning);
 | 
			
		||||
 
 | 
			
		||||
		Reference in New Issue
	
	Block a user