Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2x faster with SatoriOptions.fonts as a global variable #590

Open
jakeinflab opened this issue Feb 6, 2024 · 4 comments
Open

2x faster with SatoriOptions.fonts as a global variable #590

jakeinflab opened this issue Feb 6, 2024 · 4 comments

Comments

@jakeinflab
Copy link

Feature Request

Description

Hello everyone,

I am currently running a next.js standalone application in a production environment. Thanks to your hard work, we have been able to generate stunning OG images, greatly encouraging users to share more on social media. I am truly grateful for this.

However, serving the standalone application has introduced some constraints related to the edge runtime. As a result, we have been utilizing satori and sharp in the node.js runtime instead of relying on edge runtime and next/og.

The performance issue has been a concern, though. Combining several React components and 2-3 fonts has resulted in more delay than anticipated. Executing the following wrk command on a MacBook M1 Pro showed a performance of 10-15 RPS. The actual production environment, with less CPU resources, showed about 5 RPS:

$ wrk -t10 -c 30 -d30s http://localhost:3000/og/review\?updatedAt\=2024-01-29T14:30:22
Running 30s test @ http://localhost:3000/og/review\?updatedAt=2024-01-29T14:30:22
  10 threads and 30 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.53s   339.74ms   2.00s    66.77%
    Req/Sec     2.18      2.99    20.00     91.64%
  480 requests in 30.10s, 171.63MB read
  Socket errors: connect 0, read 0, write 0, timeout 149
Requests/sec:     15.95
Transfer/sec:      5.70MB

Adding the NODE_OPTIONS=--inspect option for profiling revealed that the addFonts() function, called during the satori constructor execution, was being repeatedly executed. (We have conducted profiling using Chrome DevTools as shown below.)

[1] Bottom-Up without grouping
image

[2] Chrome DevTools - Performance tap
image

Tracing results suggest that the bottleneck mostly occurs when calling the addFonts() function from the satori() constructor. While the actual image generation takes only about 20-50ms, 25ms is spent on addFonts(). Despite having the font buffer declared as a global variable, unnecessary operations are executed during each satori constructor call due to the processing of opentype fonts. It appears there's no interface for injecting the results of addFonts(), thus leading to the repeated execution of this function.

  • Bottleneck in addFonts() leads to event loop delays
  • results in 5-6 responses being written at once
  • lowers concurrent processing capacity.

Referencing src/fonts.ts #L135, as shown below:

  public addFonts(fontOptions: FontOptions[]) {
    // ...
    const font = opentype.parse(
        // Buffer to ArrayBuffer.
        'buffer' in data
          ? data.buffer.slice(
              data.byteOffset,
              data.byteOffset + data.byteLength
            )
          : data,
        // @ts-ignore
        { lowMemory: true }
      )
    // ...
    this.fonts.get(_name).push([font, fontOption.weight, fontOption.style])
    // ...
  }

It seems each execution of satori() assigns the result of opentype.parse() to an internal variable. I'm curious if there's a way to declare the this.fonts variable globally and either inject it from outside or reuse it in some manner.

Since we consistently use 2-3 specific fonts, minimizing the number of addFonts() calls is expected to improve throughput.

  • We manage our fonts as files and have declared them as global variables, with the intention of performing disk reads only once at the initial server startup.
  • Despite this, we suspect that frequent calls to addFonts() are causing performance delays.

This is our /pages/api/og/[id].tsx .

const [boldFontFile, semiBoldFontFile, ratingStar, inflearnLogo] =
  await Promise.all([
    getFontData("bold"),
    getFontData("semiBold"),
    readFile(path.resolve("public/images/rating_star.png")),
    readFile(path.resolve("public/images/logo.png")),
  ]);

export default async function handler(
  request: NextApiRequest,
  response: NextApiResponse
) {
  try {
    const svg = await satori(JSX,
      {
        width: 1200,
        height: 630,
        embedFont: true,
        fonts: [
          {
            name: "Pretendard",
            data: boldFontFile,
            weight: 700,
            style: "normal",
          },
          {
            name: "Pretendard",
            data: semiBoldFontFile,
            weight: 600,
            style: "normal",
          },
        ],
      }
    );

I apologize for my bad English and hope my message is clear. Please feel free to point out any imperfections in my ideas or offer any assistance you can. Thank you for taking the time to read this.

Additional Context

Maybe profiles image can be additional context.

@jakeinflab
Copy link
Author

jakeinflab commented Feb 6, 2024

TL;DR: Declaring SatoriOptions.fonts as a global variable doubled the performance

Upon closer inspection of the code, I discovered a global variable named fontCache that utilizes WeakMap. I added console.log to satori.ts#L57-L62 to check if the cache was indeed being utilized.

satori.ts#L57-L62

  let font: FontLoader
  if (fontCache.has(options.fonts)) {
    font = fontCache.get(options.fonts)
  } else {
    fontCache.set(options.fonts, (font = new FontLoader(options.fonts)))
  }

It turned out, in satori.ts, the cache key was being assigned to the memory address of the SatoriOptions.fonts object.

We had been declaring SatoriOptions.fonts as a local variable, just as the example in README.md suggests.

This made me realize that changing the address of options.fonts with every invocation prevented us from ever utilizing the cache effectively, as it just kept accumulating.

Therefore, I declared SatoriOptions.fonts as a global variable as follows:

AS-IS

export default async function handler(
  request: NextApiRequest,
  response: NextApiResponse
) {
  try {
    const svg = await satori(JSX,
      {
        width: 1200,
        height: 630,
        embedFont: true,
        fonts: [
          // ...
        ]
    );

TO-BE

const options: SatoriOptions = {
  width: 1200,
  height: 630,
  embedFont: true,
  fonts: [
    // ...
  ],
};

export default async function handler(
  request: NextApiRequest,
  response: NextApiResponse
) {
  try {
    const svg = await satori(JSX, options);
    // ...

As a result, the same code was able to handle approximately double the number of requests.

wrk -t10 -c 30 -d30s http://localhost:3000/og/review/41854\?updatedAt\=2024-01-29T14:30:22
Running 30s test @ http://localhost:3000/og/review/41854?updatedAt=2024-01-29T14:30:22
  10 threads and 30 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   992.55ms  273.04ms   1.87s    66.67%
    Req/Sec     4.43      3.97    20.00     70.93%
  891 requests in 30.07s, 318.15MB read
Requests/sec:     29.63
Transfer/sec:     10.58MB
  • AS-IS: 15.95 Requests/sec
  • TO-BE: 29.63 Requests/sec

Performance improvement by roughly 1.86 times

As you can see, this changes no longer make unnecessary calls to addFonts() in the profiling graph.

image

Now, we can see that the sendData() function, used when returning a response, occupies most of the flame graph.

image

Required

  • Modify satori.ts#L57-L62 to use serialized value as key of the map instead of the memory address value of the object
  • ImageResponse provided by @next/og might also have similar issues.

I hope this helps those experiencing performance issues. Thank you.

Regarding this, I'd like to inform @shuding , the author of this code, about the matter. If it's not too much trouble, may I go ahead and make the improvements, then submit them as a Pull Request? @shuding

@jakeinflab jakeinflab changed the title Skip addFonts() if already exists: can we inject or caching the parsed fonts? Declaring SatoriOptions.fonts as a global variable results in a 2x increase in throughput. Feb 6, 2024
@jakeinflab jakeinflab changed the title Declaring SatoriOptions.fonts as a global variable results in a 2x increase in throughput. 2x faster with SatoriOptions.fonts as a global variable Feb 14, 2024
@shuding
Copy link
Member

shuding commented Mar 4, 2024

Wow what an amazing investigation, thank you so much for your effort in this @jakeinflab!

Modify satori.ts#L57-L62 to use serialized value as key of the map

This definitely makes sense to me. Let me know if you have time to open a PR for that, I can also do it if you're busy! :thank-u:

@Zxilly
Copy link

Zxilly commented Aug 30, 2024

I'm trying to write a patch for this problem, but I'm finding it difficult to implement, the basic type isn't allowed to be the Key of a WeakMap, and the Object or Symbol is referenced differently each time it's created.
I think if we really want to implement this feature, probably we will have to convert the cache to a Map and use an algorithm like lru for example to control its size.

@Zxilly
Copy link

Zxilly commented Aug 30, 2024

Frankly, I think it's hard to be fixed within the library, and I think creating a creater and then reusing it is probably best practice in a sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants