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

[feature] Rewrite renderer to iterative algorithm #35

Merged

Conversation

scoiatael
Copy link
Contributor

Yo,

during some testing discovered that recursive algorithm sometimes breaks in the demo - this should fix it :)

I should have one more PR soon which adds support for newer PS slice format (version 7 and 8).

@scoiatael
Copy link
Contributor Author

The exact issue is that browser goes OOM and kills the process - on my rig it takes a little over 3Gb - which happens even on small (< 100Mb) designs.

@scoiatael
Copy link
Contributor Author

Identified another 20% performance improvement - hash map usage :)
Before: https://share.firefox.dev/3amFLL2
After: https://share.firefox.dev/3amFTu0

@chinedufn
Copy link
Owner

Heads up -> I'm a bit swamped at the moment. I will review this and the other PR either today or this weekend.
Thanks for diving into this!

@scoiatael
Copy link
Contributor Author

Sure, sorry for notification spam, found one 20% speedup; now: https://share.firefox.dev/3amCL1n :)

@scoiatael scoiatael force-pushed the lczaplinski/iterative-renderer branch from 6a1eb35 to 5cb52fd Compare July 7, 2022 15:02
Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

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

One small question then looks good to me! Thanks for diving into this.

@@ -159,7 +159,10 @@ pub trait IntoRgba {
let bytes_to_read = 1 + header;
for byte in cursor.read(bytes_to_read as u32) {
let rgba_idx = self.rgba_idx(idx);
rgba[rgba_idx * 4 + offset] = *byte;
let target = rgba_idx * 4 + offset;
if target < rgba.len() {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the idea behind adding this if statement?
When do you expect this case to be hit and why shouldn't we panic?

@@ -168,7 +171,10 @@ pub trait IntoRgba {
let byte = cursor.read_1()[0];
for _ in 0..repeat as usize {
let rgba_idx = self.rgba_idx(idx);
rgba[rgba_idx * 4 + offset] = byte;
let target = rgba_idx * 4 + offset;
if target < rgba.len() {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the idea behind adding this if statement?
When do you expect this case to be hit and why shouldn't we panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hit this particular problem in a couple of my test files, so this fix was part of the branch I was cherry-picking from - must've made a mistake to pull them in this PR. Will open a new one with some tests for this edge-case :) Same for the one above.

Copy link
Owner

@chinedufn chinedufn Jul 18, 2022

Choose a reason for hiding this comment

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

Woooo excited for those tests. Ok great thanks for explaining.

it was part of my testing branch and looks like I made a mistake whilst
cherry-picking. will come back with some tests for this case :)
@scoiatael
Copy link
Contributor Author

@CodeWitchBella wanna take a look?

@chinedufn chinedufn merged commit 4c3f77a into chinedufn:master Jul 18, 2022
@chinedufn
Copy link
Owner

Thanks so much for this performance improvement!

Released as 0.3.2

@scoiatael scoiatael deleted the lczaplinski/iterative-renderer branch October 11, 2022 08:27
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.

None yet

2 participants