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.

Sign up for GitHub

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

Jump to bottom

Improve performance of arrayBufferToBinaryString #3121

Merged
merged 7 commits into from
May 4, 2021
Merged

Improve performance of arrayBufferToBinaryString #3121

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
38638d7
Improve performance of arrayBufferToBinaryString
anttipalola Mar 22, 2021
d7b5b60
Change to batch fromCharCode
anttipalola Mar 24, 2021
3f2e886
fixup and comment
anttipalola Mar 24, 2021
d497fbf
improve comment yet again
anttipalola Mar 24, 2021
52a15f3
Run prettier
anttipalola Mar 24, 2021
f85fa21
Final fixes
anttipalola May 1, 2021
c724347
Merge branch 'master' into feature/arraybuffer-to-binary-string-impro…
HackbrettXXX May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/modules/addimage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ import { atob, btoa } from "../libs/AtobBtoa.js";

var UNKNOWN = "UNKNOWN";

// Heuristic limit after which String.fromCharCode will start to overflow.
// Need to change to StringDecoder.
var ARRAY_BUFFER_LIMIT = 6000000;

Copy link
Contributor

@101arrowz 101arrowz Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This figure doesn't seem right. The max frame size is 1MB to begin with, so 6MB is too big for sure. On my device, the limit is around 100kB, but I'm pretty sure going for around 16K is safe. Could you verify this in the console?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a bit large for me, too. 16k sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some benchmarks from different batch sizes: https://jsbench.me/x5kmnswnmz/1. Sweet spot at least for Chromium 89 seems to be at 4096-8192 slight degradation at 16k but slowing down rapidly after that. Manual testing shows minor GCs going to major GC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's take 8k? :)

var imageFileTypeHeaders = {
PNG: [[0x89, 0x50, 0x4e, 0x47]],
TIFF: [
Expand Down Expand Up @@ -730,20 +734,15 @@ import { atob, btoa } from "../libs/AtobBtoa.js";
var arrayBufferToBinaryString = (jsPDFAPI.__addimage__.arrayBufferToBinaryString = function(
buffer
) {
try {
return atob(btoa(String.fromCharCode.apply(null, buffer)));
} catch (e) {
if (
typeof Uint8Array !== "undefined" &&
typeof Uint8Array.prototype.reduce !== "undefined"
) {
return new Uint8Array(buffer)
.reduce(function(data, byte) {
return data.push(String.fromCharCode(byte)), data;
}, [])
.join("");
}
}
var out = "";
var u8buf = new Uint8Array(buffer);
Pantura marked this conversation as resolved.
Show resolved Hide resolved
for (var i = 0; i < u8buf.length; i += ARRAY_BUFFER_LIMIT) {
// Limit the amount of characters being parsed to prevent overflow.
// Note that while TextDecoder would be faster, it does not have the same
// functionality as fromCharCode with any provided encodings as of 3/2021.
out += String.fromCharCode.apply(null, u8buf.subarray(i, i + ARRAY_BUFFER_LIMIT));
}
return out;
});

/**
Expand Down