From 5a05db727cfae89fa0fb511191c8b28b1b5657cc Mon Sep 17 00:00:00 2001
From: Benjamin Woodruff <github@benjam.info>
Date: Mon, 26 Mar 2018 22:56:53 -0700
Subject: [PATCH] Add a per-frame limit on dynamic atlas writes

This limits the amount of damage a pathological program can cause in
thrashing the LRU cache.

I ran the script described here:
https://github.com/xtermjs/xterm.js/pull/1327#issuecomment-374003960

Without this patch, the entire output would get drawn in one frame that
took about 140 ms to draw. With this patch, it takes about 60 ms. It's
still nowhere as good as the static or none atlas implementations, but
it's not terrible, like it was.

This should have minimal impact on real-world applications: it'll just
take a little longer for the atlas to warm up.

The implementation is a little hacky, since it involves throwing some
code into TextRenderLayer that violates the separation of concerns, but
we can clean this up later.
---
 src/renderer/BaseRenderLayer.ts        |  2 +-
 src/renderer/TextRenderLayer.ts        |  2 ++
 src/renderer/atlas/BaseCharAtlas.ts    |  9 +++++++++
 src/renderer/atlas/DynamicCharAtlas.ts | 17 ++++++++++++++++-
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts
index 1747c428..a55b090d 100644
--- a/src/renderer/BaseRenderLayer.ts
+++ b/src/renderer/BaseRenderLayer.ts
@@ -20,7 +20,7 @@ export abstract class BaseRenderLayer implements IRenderLayer {
   private _scaledCharLeft: number = 0;
   private _scaledCharTop: number = 0;
 
-  private _charAtlas: BaseCharAtlas;
+  protected _charAtlas: BaseCharAtlas;
 
   constructor(
     private _container: HTMLElement,
diff --git a/src/renderer/TextRenderLayer.ts b/src/renderer/TextRenderLayer.ts
index 2487a294..edef316b 100644
--- a/src/renderer/TextRenderLayer.ts
+++ b/src/renderer/TextRenderLayer.ts
@@ -54,6 +54,8 @@ export class TextRenderLayer extends BaseRenderLayer {
       return;
     }
 
+    this._charAtlas.beginFrame();
+
     for (let y = startRow; y <= endRow; y++) {
       const row = y + terminal.buffer.ydisp;
       const line = terminal.buffer.lines.get(row);
diff --git a/src/renderer/atlas/BaseCharAtlas.ts b/src/renderer/atlas/BaseCharAtlas.ts
index 8656f212..2680bd10 100644
--- a/src/renderer/atlas/BaseCharAtlas.ts
+++ b/src/renderer/atlas/BaseCharAtlas.ts
@@ -25,6 +25,15 @@ export default abstract class BaseCharAtlas {
    */
   protected _doWarmUp(): void { }
 
+  /**
+   * Called when we start drawing a new frame.
+   *
+   * TODO: We rely on this getting called by TextRenderLayer. This should really be called by
+   * Renderer instead, but we need to make Renderer the source-of-truth for the char atlas, instead
+   * of BaseRenderLayer.
+   */
+  public beginFrame(): void { }
+
   /**
    * May be called before warmUp finishes, however it is okay for the implementation to
    * do nothing and return false in that case.
diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts
index 63da8807..7a89dac9 100644
--- a/src/renderer/atlas/DynamicCharAtlas.ts
+++ b/src/renderer/atlas/DynamicCharAtlas.ts
@@ -19,6 +19,14 @@ const TRANSPARENT_COLOR = {
   rgba: 0,
 }
 
+// Drawing to the cache is expensive: If we have to draw more than this number of glyphs to the
+// cache in a single frame, give up on trying to cache anything else, and try to finish the current
+// frame ASAP.
+//
+// This helps to limit the amount of damage a program can do when it would otherwise thrash the
+// cache.
+const DRAW_TO_CACHE_LIMIT = 100;
+
 interface IGlyphCacheValue {
   index: number;
   isEmpty: boolean;
@@ -44,6 +52,8 @@ export default class DynamicCharAtlas extends BaseCharAtlas {
   private _width: number;
   private _height: number;
 
+  private _drawToCacheCount: number = 0;
+
   constructor(document: Document, private _config: ICharAtlasConfig) {
     super();
     this._cacheCanvas = document.createElement('canvas');
@@ -69,6 +79,10 @@ export default class DynamicCharAtlas extends BaseCharAtlas {
     // document.body.appendChild(this._cacheCanvas);
   }
 
+  public beginFrame(): void {
+    this._drawToCacheCount = 0;
+  }
+
   public draw(
     ctx: CanvasRenderingContext2D,
     glyph: IGlyphIdentifier,
@@ -80,7 +94,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas {
     if (cacheValue != null) {
       this._drawFromCache(ctx, cacheValue, x, y);
       return true;
-    } else if (this._canCache(glyph)) {
+    } else if (this._canCache(glyph) && this._drawToCacheCount < DRAW_TO_CACHE_LIMIT) {
       let index;
       if (this._cacheMap.size < this._cacheMap.capacity) {
         index = this._cacheMap.size;
@@ -142,6 +156,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas {
   // TODO: We do this (or something similar) in multiple places. We should split this off
   // into a shared function.
   private _drawToCache(glyph: IGlyphIdentifier, index: number): IGlyphCacheValue {
+    this._drawToCacheCount++;
 
     // draw the background
     let backgroundColor;