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

Improve performance for concurrent conversions by re-using browser context #141

Merged
merged 2 commits into from
Nov 6, 2022

Conversation

simonhaenisch
Copy link
Owner

Replaces #139.

Closes #138.

@simonhaenisch simonhaenisch force-pushed the perf-improvements-by-reusing-browser-context branch from 5ca5133 to 0094ca7 Compare November 4, 2022 22:42
@chamabreu
Copy link
Contributor

Nice, looks cleaner.

Didn't like my approach with the passing either 👍

@chamabreu
Copy link
Contributor

Heyo,

just tested this Branch.
Does not work.
The Block

	if (!browser) {
		console.log('Have no browser, creating one');
		browser = await puppeteer.launch({ devtools: config.devtools, ...config.launch_options });
	}

Gets called many times in concurrent mode.
You have to instantiate the Browser early on, because the launch takes to long, and in the time the function gets called multiple times without a browser.

I see no other posibility than creating a browser instance at a higher level, await this, and then start the conversion processes.

As I mentioned, maybe a class based approach could help here.
What do you think?

@chamabreu
Copy link
Contributor

Heyo,

just tested this Branch. Does not work. The Block

	if (!browser) {
		console.log('Have no browser, creating one');
		browser = await puppeteer.launch({ devtools: config.devtools, ...config.launch_options });
	}

Gets called many times in concurrent mode. You have to instantiate the Browser early on, because the launch takes to long, and in the time the function gets called multiple times without a browser.

I see no other posibility than creating a browser instance at a higher level, await this, and then start the conversion processes.

As I mentioned, maybe a class based approach could help here. What do you think?

Possible Solution:
#142
I tested also class based, but this should work also.

@simonhaenisch
Copy link
Owner Author

Ah yup makes sense and that actually explains the problem I was seeing with it hanging when running concurrent tasks! Because it created a bunch of browser instances simultaneously but only closed the last one. It was too late yesterday 😅 (I started falling asleep while working on this)

I just fixed it... didn't merge your PR though because it was missing the part where we don't need to call process.exit() anymore, but set you as the commit author so you'll get the credit 😊

@simonhaenisch simonhaenisch marked this pull request as ready for review November 5, 2022 13:57
@simonhaenisch
Copy link
Owner Author

All tests passing but @chamabreu if you could test this branch one more time with your 90 files I'd be even more confident to merge this 🤓

@simonhaenisch simonhaenisch merged commit 221821d into master Nov 6, 2022
@simonhaenisch simonhaenisch deleted the perf-improvements-by-reusing-browser-context branch November 6, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: use browser context to reduce cpu load on many files
2 participants