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

Make FIND sidebar more durable #1363

Closed
wants to merge 9 commits into from
Closed

Make FIND sidebar more durable #1363

wants to merge 9 commits into from

Conversation

beru
Copy link

@beru beru commented Sep 22, 2024

This PR tackles the problem reported in #1358.

FIND sidebar gets faster and a lot tougher now.

It seems like <use> tags spend more memory compared to simply cloning <svg> nodes. So I guess they should be less often utilized for now.

I've switched to <img> tags to display the svg icons as it's much faster than using <svg> tags. Since fill and stroke properties of svg elements displayed by <img> tags cannot be controlled from outside, CSS filter is used to adjust icon colors.

I also updated the program so that Find sidebar doesn't reopen when it's already opened. This change also makes the sidebar less vulnerable to the problem.

@beru beru marked this pull request as draft September 22, 2024 07:15
@beru beru marked this pull request as ready for review September 22, 2024 08:54
@lutzroeder
Copy link
Owner

FIND sidebar gets faster and a lot tougher now.

It would help to understand why this issue is happening. e.g. is there a minimal repro or existing Chrome issue?

It seems like tags spend more memory compared to simply cloning nodes. So I guess they should be less often utilized for now.

Start with a smaller less risky pull request to implementing cloning only?

I've switched to tags to display the svg icons as it's much faster than using tags. Since fill and stroke properties of svg elements displayed by

Multiple .svg files will lead to extra downloads. Filter will make this hard to maintain, cause issues with theming if other colors are introduced. Is there a way to measure the before and after impact? Virtualization might be a better solution with fewer downsides?

I also updated the program so that Find sidebar doesn't reopen when it's already opened. This change also makes the sidebar less vulnerable to the problem.

Rebase to main. Checking english strings can lead to issues with localization.

@beru
Copy link
Author

beru commented Sep 22, 2024

e.g. is there a minimal repro or existing Chrome issue?

I searched Chromium issues but I couldn't find a corresponding ticket. Maybe I don't have good searching skills.
https://issues.chromium.org/issues?q=%22cppgc::LivenessBroker::IsHeapObjectAliveImpl%22
https://issues.chromium.org/issues?q=%22Ran%20out%20of%20reservation%22

I'll see if I can create a minimal html file that would reproduce the problem. It it goes well, I'll create an issue at https://issues.chromium.org

Start with a smaller less risky pull request to implementing cloning only?

The first commit (b2d5154) does it. But more recent commit (937341f) that uses <img src=".svg"> tags is faster and stable.

Multiple .svg files will lead to extra downloads.

It looks like using external .svg files can be avoided with <img src="data:image/svg+xml;utf8, <svg..... tags. I didn't know this, I thought it wasn't possible.
https://stackoverflow.com/a/28379733
(edited: svg elements without indentation are hard to read/write though.)

Filter will make this hard to maintain, cause issues with theming if other colors are introduced.

Another idea I can think of is replacing template img tag's src text (color part) with JavaScript after checking current color-scheme.
https://stackoverflow.com/a/57795495

Is there a way to measure the before and after impact?

I still haven't automated testing this. I simply run the app with VS Code (Electron View) and check how it performs using a pair of hands and eyes.

I've used efficientdet-d3.onnx file to check FIND sidebar opening speed. On my machine it originally took about 4.5 sec to do so. <svg> cloneNode version reduces the time to around 3.5 sec. And it takes about 2 sec with <img src=".svg"> version.

To check stability of the program (#1358), I also use efficientdet-d3.onnx file. After opening the file, I type Ctrl+F and Esc keys alternately to open and close FIND sidebar. Repeating this reproduces the bug with original code. I think the latest commit of this PR has almost solved the problem.

Virtualization might be a better solution with fewer downsides?

List Virtualization would be a lot faster since it uses far less elements. It would also solve the problem reported in #1358. Since the number of items can be quite high with some models, I'd say it is imperative that Netron deploys List Virtualization.

@beru beru marked this pull request as draft September 22, 2024 19:07
@lutzroeder
Copy link
Owner

The first commit does it. But more recent commit that uses tags is faster and stable.

Can you validate the change at TIP which uses cloneNode. Leaning towards not adopting the <img> approach as it will complicate maintenance and virtualization might make this unnecessary.

List Virtualization would be a lot faster since it uses far less elements. It would also solve the problem reported

Yes, right course of action seems to help getting the root issue in Chrome investigated. We could then pivot #1358 to "Find sidebar virtualization" if that's a more promising long-term solution.

@lutzroeder lutzroeder closed this Sep 22, 2024
@beru
Copy link
Author

beru commented Sep 23, 2024

Can you validate the change at TIP which uses cloneNode.

I've just checked the latest commit (f4d88dc).

I can still reproduce the problem reported in #1358 by quickly opening/closing the FIND sidebar (with efficientdet-d3.onnx file). But I think in normal usage, this doesn't reproduce when users operate at a moderate speed. So I'd like to think this is not an urgent issue.

As for the opening speed of the sidebar. It is probally the same. Inlining svg content is actually faster than <use> tag. https://cloudfour.com/thinks/svg-icon-stress-test/#inline-svg Since contents of the icons are very simple, I recommend this way. Also, <span> elements inside <li> list items can be reduced. These changes (beru@038accd) help to reduce a little bit of time, not significant though.

Leaning towards not adopting the approach as it will complicate maintenance and virtualization might make this unnecessary.

Surely the <img> approach is difficult to maintain and it's not super fast so it's not worth sacrificing the sanity of developers.

@beru beru deleted the its_no_use branch September 23, 2024 02:05
lutzroeder added a commit that referenced this pull request Sep 24, 2024
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.

2 participants