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

Optimize Schematic.Deserialize, a lot #189

Merged
merged 11 commits into from
May 7, 2023
Merged

Conversation

dsharlet
Copy link
Owner

@dsharlet dsharlet commented May 6, 2023

This may work around the issue raised in #187. Schematic.Deserialize should be orders of magnitude faster with this branch.

The problem is when deserializing a circuit, the components and wires would be added as if they were added manually in the UI, rebuilding the nodes one at a time. The ConnectedTo algorithm was also quadratic. So I think the old behavior was two quadratic algorithms nested inside each other. How embarrassing :)

Copy link
Collaborator

@Federerer Federerer left a comment

Choose a reason for hiding this comment

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

From over 3.2 seconds to around 30 milliseconds, that's really a lot 😉 I remember looking at this, when I've been testing why move tool is so slow. This however, will still be a problem, as the move tool generates an event for every element. Using some kind of debounce there helps a lot, but I didn't wanted to add a dependency on System.Reactive in Circuit library, because it's quite heavy and introduces a lot of unwanted dependencies in netstandard projects, see: dotnet/reactive#1847 so we might need to implement something just for this use-case.

@dsharlet
Copy link
Owner Author

dsharlet commented May 6, 2023

Yeah, I was thinking about that too. I think that is fixable also by batching the node rebuilds, just kinda annoying to actually implement somehow. I started down that route before just adding a special case for the constructor, but we should probably just do both.

@dsharlet
Copy link
Owner Author

dsharlet commented May 6, 2023

Enabling batching is actually a massive headache because of the way I designed all this. I'm now just trying to optimize everything that happens while moving a large selection around. I think it should be possible to make it much faster.

@Federerer
Copy link
Collaborator

Enabling batching is actually a massive headache because of the way I designed all this.

I know, I've tried doing it this way and gave up. Can't remember though why I dropped this concept and switched to debounce 🤔

@dsharlet
Copy link
Owner Author

dsharlet commented May 7, 2023

Well, I wouldn't say moving around a lot of elements at once is fast now, but it's a lot better than it was. I can move around a large circuit all at once, and it at least feels pretty responsive, but it still makes my CPU fan go nuts :)

@dsharlet
Copy link
Owner Author

dsharlet commented May 7, 2023

I think at this point there isn't much room for optimization without major design refactoring. There's basically just one major issue left, that we have to reconnect all terminals whenever a wire moves. Batching wire moves would of course make this better, but fixing either of these issues is probably more trouble than it's worth. Moving an entire large circuit around at least doesn't feel really laggy any more.

@dsharlet dsharlet mentioned this pull request May 7, 2023
@dsharlet
Copy link
Owner Author

dsharlet commented May 7, 2023

I was mistaken, this is now legitimately fast. According to the profiler, now more time is spent in WPF drawing and such, than schematic manipulation. I had been trying to use bounding boxes to optimize this the whole time, but it turns out there was a boneheaded bug in that code. Fixing that makes it work as expected.

@dsharlet
Copy link
Owner Author

dsharlet commented May 7, 2023

It's quite likely this is introducing some bugs. I've found and fixed a few (including a few old ones from before this change), but I'm just going to merge it and keep working on and testing other things.

@dsharlet dsharlet merged commit edb1aad into master May 7, 2023
@dsharlet dsharlet deleted the optimize-deserialize branch May 7, 2023 20:32
@Federerer
Copy link
Collaborator

Nice 😁 I'll check this tomorrow.

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.

Schematic cannot be displayed in multiple editors simultaneously.
2 participants