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

if cache pack is too big, we should batch writing #15373

Merged
merged 4 commits into from Feb 15, 2022
Merged

if cache pack is too big, we should batch writing #15373

merged 4 commits into from Feb 15, 2022

Conversation

vankop
Copy link
Member

@vankop vankop commented Feb 11, 2022

What kind of change does this PR introduce?

fixes #14907

Did you add tests for your changes?

there are existing tests in #15367

Does this PR introduce a breaking change?

no

What needs to be documented once your changes are merged?

nothing

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@vankop vankop mentioned this pull request Feb 11, 2022
@vankop
Copy link
Member Author

vankop commented Feb 11, 2022

previous version

const batchWrite = err => {
	if (i === len) {
		stream.end();
		return;
	}
	// will be handle in "on" handler
	if (err) return;
	const start = i;
	let batchSize = 0;
	while (i < len && batchSize < _1_5_GB)
		batchSize += content[i++].length;
	return stream.write(
		Buffer.concat(content.slice(start, i), batchSize),
		batchWrite
	);
};

hm.. seems like

const batchWrite = err => {
	if (i === len) {
		stream.end();
		return;
	}
	// will be handle in "on" handler
	if (err) return;
	stream.write(content[i++], batchWrite);
};

is faster... I have checked I/O and data + number of I/O per sec seems roughly equal
Screenshot 2022-02-11 at 15 55 14
( red lined batched version )
maybe this will be better I decided to use this version

if cache pack is too big, we should write in stream using callback
* chunks of 511MiB and
* total of 2GiB
for (const b of content) stream.write(b);
stream.end();
// split into chunks for WRITE_LIMIT_CHUNK size
const chunks = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

if (size < WRITE_LIMIT_CHUNK) {
   for (const b of content) stream.write(b);
   return stream.end();
}

I think we still can use this

Copy link
Member

Choose a reason for hiding this comment

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

We could, but that doesn't really makes a difference...

Copy link
Member Author

Choose a reason for hiding this comment

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

for small builds we will skip chunking..

@sokra sokra merged commit ba4e83c into main Feb 15, 2022
@sokra sokra deleted the fix/issue-14907 branch February 15, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persistent caching errors when a packfile is > 2GB
3 participants