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

Reduce Allocations for GraphicsGroup Draw Calls #3284

Open
blakearoberts opened this issue Nov 23, 2024 · 1 comment · May be fixed by #3285
Open

Reduce Allocations for GraphicsGroup Draw Calls #3284

blakearoberts opened this issue Nov 23, 2024 · 1 comment · May be fixed by #3285
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior

Comments

@blakearoberts
Copy link

Context

I am trying to render thousands of tiles to the screen. I use a GraphicsGroup to "chunk" draw calls. If I profile a static scene, the JS heap will grow before performing a GC that impacts frame rate:

image

I did a memory allocation profile over 15 seconds, and get localBounds allocated about 60 MB.

Looking at that method, I notice a few allocations:

  • let bb = new BoundingBox();
  • bb = member.localBounds.combine(bb);
  • bb = graphic.localBounds.translate(pos).combine(bb);

Proposal

Is it possible to reduce the number of allocations of get localBounds by this.members.length by reusing bb in combine?

public get localBounds(): BoundingBox {
  let bb = new BoundingBox();
  for (const member of this.members) {
    if (member instanceof Graphic) {
      member.localBounds.combine(bb, bb); // use bb as dest to avoid allocation
    } else {
      const { graphic, offset: pos, useBounds } = member;
      const shouldUseBounds = useBounds === undefined ? true : useBounds;
      if (graphic) {
        if (shouldUseBounds) {
          graphic.localBounds.translate(pos).combine(bb, bb); // use bb as dest to avoid allocation
        }
      } else {
        this._logger.warnOnce(`Graphics group member has an null or undefined graphic, member definition: ${JSON.stringify(member)}.`);
      }
    }
  }
  return bb;
}
@eonarheim
Copy link
Member

@blakearoberts Great suggestion! Let's do it, 1 allocation over many is way better

@eonarheim eonarheim added the bug This issue describes undesirable, incorrect, or unexpected behavior label Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants