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

Do not create a new TextDecoder for every record #1031

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

achim-k
Copy link
Contributor

@achim-k achim-k commented Dec 5, 2023

Public-Facing Changes

Do not create a new TextDecoder for every record

Description

Massively improves read performance by avoiding unnecessary creation / cleanups of TextDecoders.

Before:

 [Summary]:
   ticks  total  nonlib   name
   6600    7.2%   46.1%  JavaScript
   7693    8.4%   53.8%  C++
  21411   23.3%  149.6%  GC
  77489   84.4%          Shared libraries
     16    0.0%          Unaccounted

After (Notice the drop in GC time):

 [Summary]:
   ticks  total  nonlib   name
   3950   10.1%   60.7%  JavaScript
   2555    6.6%   39.2%  C++
    549    1.4%    8.4%  GC
  32426   83.3%          Shared libraries
      5    0.0%          Unaccounted

@@ -1,9 +1,10 @@
import { getBigUint64 } from "./getBigUint64";

const textDecoder = new TextDecoder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a comment about why we are using a global decoder.

I'm not aware of any issue with doing this (I've long hated that the API requires a new instance each time) but it could be that the decoder holds onto some internal state for longer than we'd want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment. The encoding seems to be the TextDecoder's only internal state

@achim-k achim-k merged commit b6ea171 into main Dec 5, 2023
24 checks passed
@achim-k achim-k deleted the achim/use-single-text-decoder branch December 5, 2023 22:18
pezy pushed a commit to pezy/mcap that referenced this pull request Jan 11, 2024
### Public-Facing Changes

Do not create a new TextDecoder for every record

### Description
Massively improves read performance by avoiding unnecessary creation /
cleanups of TextDecoders.

Before:
```
 [Summary]:
   ticks  total  nonlib   name
   6600    7.2%   46.1%  JavaScript
   7693    8.4%   53.8%  C++
  21411   23.3%  149.6%  GC
  77489   84.4%          Shared libraries
     16    0.0%          Unaccounted
```

After (Notice the drop in GC time):
```
 [Summary]:
   ticks  total  nonlib   name
   3950   10.1%   60.7%  JavaScript
   2555    6.6%   39.2%  C++
    549    1.4%    8.4%  GC
  32426   83.3%          Shared libraries
      5    0.0%          Unaccounted
```
pezy pushed a commit to pezy/studio that referenced this pull request Sep 14, 2024
**User-Facing Changes**
Improve performance of reading MCAP files

**Description**
Includes foxglove/mcap#1031
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants