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
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index 8ca4f9d..cc66e3b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -76,6 +76,9 @@
// 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 @@
this.dispatchEvent(new CustomEvent('render-content',
{bubbles: true}));
- if (this._anyLineTooLong()) {
+ if (this._diffTooLargeForSyntax()) {
this.$.syntaxLayer.enabled = false;
}
@@ -473,10 +476,32 @@
}, 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>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 7f1450c..4d7b821 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -1052,6 +1052,10 @@
});
});
+ test('getDiffLength', () => {
+ assert.equal(element.getDiffLength(diff), 52);
+ });
+
test('getContentByLine', () => {
let actual;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 6add75c..bddfc6d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -310,7 +310,7 @@
<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
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 9f1b412..3ae8806 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -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();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 0274fae..faf529b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -55,12 +55,6 @@
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 @@
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 @@
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 @@
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 @@
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);
