-
-
Notifications
You must be signed in to change notification settings - Fork 296
Don't store resources content in memory - stream to storage (fixes #386) #643
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
base: master
Are you sure you want to change the base?
Changes from all commits
6deb6cf
9585992
7395e61
74bc674
a3c6754
ace35a4
420a146
17c58a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| config_version = "0" | ||
|
|
||
| exclude_patterns = [ | ||
| "test/**", | ||
| ] | ||
|
|
||
| [coverage] | ||
| ignores = [ | ||
| "benchmark/**", | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| /* | ||
| * Memory benchmark for issue #386. | ||
| * | ||
| * Starts a local http server serving an html page referencing FILES_COUNT | ||
| * binary files of FILE_SIZE_MB each (generated chunk-wise on the fly, so the | ||
| * server itself stays flat), scrapes it and reports peak memory usage. | ||
| * | ||
| * Usage: | ||
| * npm run benchmark:memory | ||
| * FILES_COUNT=50 FILE_SIZE_MB=10 REQUEST_CONCURRENCY=8 npm run benchmark:memory | ||
| */ | ||
|
|
||
| import http from 'http'; | ||
| import fs from 'fs'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
| import scrape from 'website-scraper'; | ||
|
|
||
| const FILES_COUNT = parseInt(process.env.FILES_COUNT || '100', 10); | ||
| const FILE_SIZE_MB = parseInt(process.env.FILE_SIZE_MB || '20', 10); | ||
| const REQUEST_CONCURRENCY = parseInt(process.env.REQUEST_CONCURRENCY || '8', 10); | ||
|
|
||
| const CHUNK = Buffer.alloc(64 * 1024, 0xab); | ||
| const CHUNKS_PER_FILE = Math.ceil(FILE_SIZE_MB * 1024 * 1024 / CHUNK.length); | ||
|
|
||
| function startServer () { | ||
| const server = http.createServer((req, res) => { | ||
| if (req.url === '/') { | ||
| const imgs = Array.from({length: FILES_COUNT}, (_, i) => `<img src="blob/${i}.bin">`).join('\n'); | ||
| res.writeHead(200, {'content-type': 'text/html'}); | ||
| res.end(`<html><head></head><body>${imgs}</body></html>`); | ||
| return; | ||
| } | ||
|
|
||
| res.writeHead(200, {'content-type': 'application/octet-stream'}); | ||
| let sent = 0; | ||
| const writeChunk = () => { | ||
| while (sent < CHUNKS_PER_FILE) { | ||
| sent++; | ||
| if (!res.write(CHUNK)) { | ||
| res.once('drain', writeChunk); | ||
| return; | ||
| } | ||
| } | ||
| res.end(); | ||
| }; | ||
| writeChunk(); | ||
| }); | ||
|
|
||
| return new Promise((resolve) => { | ||
| server.listen(0, '127.0.0.1', () => resolve(server)); | ||
| }); | ||
| } | ||
|
|
||
| function formatMB (bytes) { | ||
| return `${Math.round(bytes / 1024 / 1024)} MB`; | ||
| } | ||
|
|
||
| const server = await startServer(); | ||
| const baseUrl = `http://127.0.0.1:${server.address().port}/`; | ||
| const directory = fs.mkdtempSync(path.join(os.tmpdir(), 'website-scraper-benchmark-')); | ||
| fs.rmSync(directory, {recursive: true, force: true}); // scraper requires the directory to not exist | ||
|
|
||
| const peak = {rss: 0, heapUsed: 0, external: 0, arrayBuffers: 0}; | ||
| const sampler = setInterval(() => { | ||
| const usage = process.memoryUsage(); | ||
| for (const key of Object.keys(peak)) { | ||
| peak[key] = Math.max(peak[key], usage[key]); | ||
| } | ||
| }, 200); | ||
|
|
||
| console.log(`scraping ${FILES_COUNT} files x ${FILE_SIZE_MB} MB (${FILES_COUNT * FILE_SIZE_MB} MB total), concurrency ${REQUEST_CONCURRENCY}`); | ||
| const startedAt = Date.now(); | ||
|
|
||
| try { | ||
| await scrape({ | ||
| urls: [baseUrl], | ||
| directory, | ||
| requestConcurrency: REQUEST_CONCURRENCY | ||
| }); | ||
| } finally { | ||
| clearInterval(sampler); | ||
| server.close(); | ||
| fs.rmSync(directory, {recursive: true, force: true}); | ||
| } | ||
|
|
||
| console.log(`done in ${((Date.now() - startedAt) / 1000).toFixed(1)}s`); | ||
| console.log(`peak rss: ${formatMB(peak.rss)}`); | ||
| console.log(`peak heapUsed: ${formatMB(peak.heapUsed)}`); | ||
| console.log(`peak external: ${formatMB(peak.external)}`); | ||
| console.log(`peak arrayBuffers: ${formatMB(peak.arrayBuffers)}`); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import path from 'path'; | ||
| import fs from 'fs'; | ||
| import { outputFile } from '../utils/fs.js'; | ||
| import { outputFileStream } from '../utils/fs.js'; | ||
|
|
||
| class SaveResourceToFileSystemPlugin { | ||
| apply (registerAction) { | ||
|
|
@@ -20,8 +20,12 @@ class SaveResourceToFileSystemPlugin { | |
|
|
||
| registerAction('saveResource', async ({resource}) => { | ||
| const filename = path.join(absoluteDirectoryPath, resource.getFilename()); | ||
| const text = resource.getText(); | ||
| await outputFile(filename, text, resource.getEncoding()); | ||
| try { | ||
| await outputFileStream(filename, resource.getContentStream()); | ||
| } catch (err) { | ||
| await fs.promises.rm(filename, {force: true}); // don't leave partially written files | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [correctness] The cleanup
Wrapping the |
||
| throw err; | ||
| } | ||
| loadedResources.push(resource); | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness] Mid-stream failure on the first resource leaves the output directory behind, breaking retries
outputFileStreamrunsfs.mkdir(dir, {recursive: true})before piping, so the directory tree exists before the first resource completes. If the connection drops mid-body:catchbelow removes only the partial file, not the directories it created;erroraction (line 32) skipsrm(absoluteDirectoryPath)becauseloadedResourcesis still empty.In v6 the body was fully buffered before any fs write, so a network error left no directory. Now
scrape()rejects, the empty output directory remains, and retrying the exact same call fails immediately inbeforeStartwithDirectory ... exists— manual cleanup required.Dropping the
loadedResources.length > 0gate (rm withforce: trueis a no-op if the dir was never created) would fix the retry regression.