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 the Wafer Map web worker class to support offscreen rendering #1929

Conversation

Razvan1928
Copy link
Contributor

@Razvan1928 Razvan1928 commented Mar 13, 2024

Pull Request

🤨 Rationale

Wafer Map's canvas is the main component used to draw the actual dies on a wafer

image

The old wafer renders the canvas on the main thread, for the new one we want to render the canvas inside the web worker

👩‍💻 Implementation

The main changes were done in matrix-renderer.ts, worker-renderer.ts and index.ts
matrix-renderer.ts - holds the logic on how the canvas drawn, most of the changes are straightforward besides the typed array parsing algorithm from drawWafer(), but a comment explaining it can be found there
worker-renderer.ts - holds the logic on how worker is setup
index.ts - creates the worker and transfers the canvas control from main thread to the worker

less important changes were done in WaferMapTests.ts from the Blazor component and zoom-handler.ts
WaferMapTests.ts - after adding a new wafer map tag inside the wafermap template.ts the blazor tests were not able to distinguise between the 2 tags, I added IDs to each tag and changed the page.Locator to search for the ID and not for the tag name
zoom-handler.ts - zoom behavior was managed inside update() method from index.ts, as the constructor of ZoomHandler takes an wafer map as input we implemented the observable design pattern using Notifier from Fast Element, similar to how it is done for table nimble component

🧪 Testing

Added a few unit tests and a chromatic test

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@Razvan1928 Razvan1928 requested a review from munteannatan March 13, 2024 12:30
@Razvan1928 Razvan1928 changed the title Users/rnazare/improve the web worker class to support offscreen rendering Improve the web worker class to support offscreen rendering Mar 13, 2024
@Razvan1928 Razvan1928 changed the title Improve the web worker class to support offscreen rendering Improve the Wafer Map web worker class to support offscreen rendering Mar 13, 2024
…e/Improve-the-web-worker-class-to-support-offscreen-rendering
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Initial feedback, still need to review some bits

@Razvan1928 Razvan1928 requested a review from rajsite April 24, 2024 15:46
@Razvan1928 Razvan1928 requested a review from rajsite April 25, 2024 14:59
this.wafermap.experimentalDataManager.margin
);

if (this.wafermap.diesTable !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I think all these checks for diesTable are undefined are incorrect because they result in the worker not receiving any message.

But the user could intentionally be trying to clear the current wafer that is being drawn. Now if you set diesTable to undefined nothing happens right (the wafer canvas stays the same even though the data was cleared)?

Maybe we are being too clever with the diesTable property and renderer. We should consider adding a boolean property like use-experimental-renderer and just be explicit for the user to opt-in.

I think there are two outcomes and they should not block this PR:

  • setting and clearing the diesTable should behave well
  • (up to y'all but I think it will help) we should make an explicit boolean configuration to switch rendering of the wafer map.

If that makes sense can you track it under Render Dies in the tracking issue? #2034

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we have a flag on the wafer-map called isExperimentalRenderer() but it is checked before calling setupWafer(), inside update() from index.ts there is a
if (this.isExperimentalUpdate()) { this.currentTask = this.experimentalUpdate(); return; }
and the call stack is update() -> this.experimentalUpdate() -> this.workerRenderer.setupWafer();

so we can assume every time this.workerRenderer.setupWafer it is an experimental wafer

Regarding the undefined checking and the worker not receiveing messages, I replaced that check with
image
now the wafer is cleared when diesTable is undefined

Copy link
Member

Choose a reason for hiding this comment

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

Reopening this question. @Razvan1928

  • isExperimentalUpdate just checks that diesTable is undefined, are you sure that is sufficient? Did you do any manual verification?
  • are there tests to verify the behavior change? Ie the wafer if drawn, then the data is changed to undefined, and the wafer is cleared?

If there are not tests there needs to be a follow-up PR

@Razvan1928 Razvan1928 merged commit a3865cb into main Apr 26, 2024
13 checks passed
@Razvan1928 Razvan1928 deleted the users/rnazare/Improve-the-web-worker-class-to-support-offscreen-rendering branch April 26, 2024 07:55
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.

6 participants