brownplt / code.pyret.org

Website for serving Pyret to folks.
Other
24 stars 45 forks source link

_vertices array for images considered harmful #362

Open blerner opened 3 years ago

blerner commented 3 years ago

Related to https://github.com/brownplt/code.pyret.org/issues/361, the _vertices array on images is a source of quadratic or worse memory bloat. Worse, the following code causes Chrome to crash CPO with a stack error, even though there's no actual stack unsafety present:

include image

fun fractal(depth):
  if depth == 0:
    rectangle(2, 2, "solid", "red")
  else:
    f = fractal(depth - 1)
    above(f, beside(f, f))
  end
end

fractal(9) # Raise this to 10 to crash

I'm being nice by memoizing the nested fractal calls, but the outermost above image results in an image whose _vertices array contains nearly 79K vertices (4 * 3^9), from concatenating all the vertices of each rectangle over and over and over again, even though there are only 19 images allocated in this computation (1 RectangleImage and 18 OverlayImages). If I didn't memoize, then we'd also have 79K RectangleImages and ~19K OverlayImages to deal with, further raising the memory pressure.

I don't think we need the _vertices array, and since they're only used for bounding-box computations which we can (and probably should) compute differently anyway, I think we should eliminate this.

schanzer commented 3 years ago

I finally had a chance to dig through the WeScheme codebase (which is more familiar to me). You're right -- we do not need this array. At one point I had designs on it being a part of some reconciliation algorithm akin to what Robby does in Racket, but it only wound up serving as an over-engineered solution to a much simpler problem (computing bounding boxes). I say kill it. Go with god.