Skip to content
This repository has been archived by the owner on Mar 2, 2024. It is now read-only.

[perf] A lot of time is spent on getSvgPathFromStroke when panning #116

Closed
TodePond opened this issue Sep 7, 2022 · 19 comments
Closed

[perf] A lot of time is spent on getSvgPathFromStroke when panning #116

TodePond opened this issue Sep 7, 2022 · 19 comments
Labels
bug Something isn't working

Comments

@TodePond
Copy link
Contributor

TodePond commented Sep 7, 2022

image

This seems to be true:
image

I recorded that when panning around this:
image

In some frames, it's the longest part:
image

I feel like... for panning, the result of this function could be cached?
It would probably be useful to have a standardised way of benchmarking this, first of all. I might get round to it at some point!

@TodePond TodePond added the bug Something isn't working label Sep 7, 2022
@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

I'll use this file to run the benchmarks on: https://gist.githubusercontent.com/TodePond/16813d281d989ca14a291a6d662abc75/raw/ecf4187c2259e5f7824d541d39f2f5bdb01ad4c0/benchmark-panning.tldr

It seems to be enough to slow down panning speed on my desktop due to the getSvgPathFromStroke function (when you're zoomed in far enough):
image

This is what it should look like:
image

Note: Some other stuff is also causing frame-drops when zoomed out, but I won't focus on those in this issue:
image

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

Ok I whipped up a little benchmark tool on this branch: https://github.com/TodePond/tldraw/tree/benchmark-panning

It's available from the dev menu:
image

When the page loads, it loads a heavy file and pans the camera around a bit.

It overrides getSvgPathFromStroke with whatever candidate you want to test (and it injects in some measuring too).
I think that should properly test it, but not entirely sure.

I've made the final score output a TOTAL time instead of AVERAGE time because there might be scope to reduce the number of getSvgPathFromStroke calls in the first place.

Next up: I'm gonna test some of the ideas from the twitter thread and make a spreadsheet.

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

I've started measuring different candidates:
image

The two examples refer to the two functions in this codesandbox (except, I haven't added testing for the new strokePoints style of function yet).
I'll update my branch to the updated function, and then measure all the different suggestions

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

Some very minor but consistent-ish speedup from interpolating strings:
image
image

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

Doing this suggestion slowed it down on my machines:
image
image

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

Here is the spreadsheet: https://docs.google.com/spreadsheets/d/1EGla00vZ-iXbfy9AqipBMlmA7XwY2IAPp5IvDiN1T2o/edit?usp=sharing

I've added some different things that I'm planning to try

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

Removing all toFixed(3) calls increased speed by quite a lot, suggesting that it's a decent area to focus on:

image

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

I did a proof-of-concept experiment where a shape's fill path is cached, and it sped things up by quite a bit. If this was cleaned up, it could help! (currently it breaks some things)

image

I did this sort of messy thing (in this branch: https://github.com/TodePond/tldraw/tree/benchmark-panning-cache)
image

I think I'll pause that there for now, and maybe come back to clean up all of this another time!

@TodePond
Copy link
Contributor Author

TodePond commented Sep 7, 2022

It's even quicker if you get rid of the toFixed(3) calls, but I guess that comes at some other cost too?

image

@TodePond
Copy link
Contributor Author

Recent developments: steveruizok/perfect-freehand#56

I'll benchmark those changes later this week :)

@chenglou
Copy link

What's "Function call" btw? That seems like the bigger bottleneck.
Caches would speed up your benchmarks for sure, but would slow down atypical cases. I'd suggest reaching for that last. It'd just make other operations, such as batch resize, even slower (unless you also put some complex caching to that, and to another, and to another).

@TodePond
Copy link
Contributor Author

hello :)

I'll have a look later today at that function call!

but would slow down atypical cases

let's measure it :D If I get time this week, I'd love to measure those atypical cases.

lemme think...

  • resizing a lot of things at the same time
  • anything else I wonder 🤔

awesome work btw! I learnt something new about svgs from your PR ♥️

@TodePond
Copy link
Contributor Author

TodePond commented Sep 19, 2022

What's "Function call" btw? That seems like the bigger bottleneck.

I checked, and 'Function call' is just something to do with loading the page in the first place.
When only profiling after full load, the self-time of it shrinks a lot.

This is when we pan while zoomed out (so all strokes are visible):
image

And this is when we pan while zoomed in (so strokes go in-and-out of visibility) (BASELINE is getSvgPathFromStroke):
image

@TodePond
Copy link
Contributor Author

So far, in my measurements, I'm struggling to find a speed-increase from changing toFixed(3) to toFixed(2)

I'll revisit it later to measure it when combined with the other changes.

image

@TodePond
Copy link
Contributor Author

My first attempt at implementing the 'magic' change:
image

I enjoy these happy little accidents! I'll try to figure out what I did wrong.

Here's my attempt at translating it over to the number[][] format:

function getSvgPathFromStroke(points: number[][]): string {
    const len = points.length

    // TODO: support smaller lengths - but just ignore for now while we measure
    if (len < 3) {
      return ''
    }

    const first = points[0]
    const second = points[1]
    const third = points[2]
    let x0 = first[0]
    let y0 = first[1]
    let x1 = second[0]
    let y1 = second[1]
    let x2 = third[0]
    let y2 = third[1]

    let result = `M${x0.toFixed(3)},${y0.toFixed(3)} Q ${x1.toFixed(3)},${y1.toFixed(3)} ${average(
      x1,
      x2
    ).toFixed(3)},${average(y1, y2).toFixed(3)}
      T `

    for (let i = 0, max = len - 1; i < max; i++) {
      // TODO: bound check, start at > 0, etc.
      result += `${average(points[i][0], points[i + 1][0]).toFixed(3)},${average(
        points[i][1],
        points[i + 1][1]
      ).toFixed(3)} `
    }

    result += 'Z'

    return result
  }

@TodePond
Copy link
Contributor Author

TodePond commented Sep 19, 2022

Hmm, I still can't get it working. I've tried to copy-paste the 'magic' code as much as I can, and I get a fun wobbly line:
image

This function is not intended to be performant - it is just intended to check that the code is creating the svg as intended:

function getSvgPathFromStroke(points: number[][]): string {
    
    const xs = points.map((point) => point[0])
    const ys = points.map((point) => point[1])

    const len = xs.length
  
    // TODO: support smaller lengths - but just ignore for now while we measure
    if (len < 3) {
      return ''
    }
  
    let x0 = xs[0], y0 = ys[0], x1 = xs[1], y1 = ys[1], x2 = xs[2], y2 = ys[2]
    let result = `M${x0.toFixed(2)},${y0.toFixed(2)}
      Q ${x1.toFixed(2)},${y1.toFixed(2)} ${average(x1, x2).toFixed(2)},${average(y1, y2).toFixed(2)}
      T `;
  
    for (let i = 0, max = len - 1; i < max; i++) { // TODO: bound check, start at > 0, etc.
      result += `${average(xs[i], xs[i+1]).toFixed(2)},${average(ys[i], ys[i+1]).toFixed(2)} `;
    }
  
    result += 'Z'
  
    return result
  }

I'll have a break and come back to this with fresh eyes another time :)

@TodePond
Copy link
Contributor Author

@steveruizok provided a fix!

The 'magic' approach is very fast!

image

@TodePond
Copy link
Contributor Author

I'll close this now because of the recent change, but I'll keep doing some benchmarks :)

@TodePond
Copy link
Contributor Author

I added some extra test candidates. MAGIC_NO_FIXED is the current fastest, but comes with a cost (it uses the magic function, and removes all toFixed calls).

image

@TodePond TodePond transferred this issue from tldraw/tldraw May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants