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

feat: add basic render queueing #6851

Merged
merged 7 commits into from
Mar 1, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Feb 16, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #6130
Work on #6786

Proposed Changes

Adds a render queue that is used to delay / batch block rerendering. See #6786 for info about what exactly this means

Reason for Changes

5x performance improvement!

Test Coverage

Only manual testing.

Documentation

N/A

Additional Information

There are some things in core that need blocks to be rendered immediately. E.g. flyouts need blocks to render immediately so that they can then be positioned. So I only converted dragging / connecting / disconnecting to use the queue, and we can convert other things over time.

This did run into some problems with the connection database, because rendering blocks after a delay means their connections have moved a bit before the normal call to tighten happens. Which is why we need to updateConnectionLocations on all blocks, even the ones that haven't been rendered. So we might need to pull this change out if it causes other unfound problems related to that.

On the other hand, I think our connection tracking is badly understood and overly complex. So I might look into cleaning this up a bit. E.g. why does tighten look at the relativeXY of the block when it can just calculate the offset based on the offsetInBlock_ of it and its parent connection?

On the other other hand, touching the connection stuff is scary :P

@ewpatton
Copy link
Contributor

@BeksOmega We implemented something similar in App Inventor. One additional challenge we ran into was dealing with the connection database where it devolves into insertion sort on large updates to the workspace (such as clean up blocks), so we also included a mechanism to defer updating the DB and then used the built-in sorting capabilities which are O(n log n).

@BeksOmega
Copy link
Collaborator Author

@BeksOmega We implemented something similar in App Inventor. One additional challenge we ran into was dealing with the connection database where it devolves into insertion sort on large updates to the workspace (such as clean up blocks), so we also included a mechanism to defer updating the DB and then used the built-in sorting capabilities which are O(n log n).

Ooo good idea!! I'll have to see if that's workable here :D

@BeksOmega
Copy link
Collaborator Author

@BeksOmega We implemented something similar in App Inventor. One additional challenge we ran into was dealing with the connection database where it devolves into insertion sort on large updates to the workspace (such as clean up blocks), so we also included a mechanism to defer updating the DB and then used the built-in sorting capabilities which are O(n log n).

So I was looking into this some more, and I'm confused about how it devolves into insertion sort. splice is constant for low numbers. calculateIndexForYPos_ is O(log n). findIndexOfConnection_ does have a linear component, but if your workspace is distributed vertically, it should essentially be O(log n). So in general, I think reconstructing the database from scratch should be O(n log n) using the current algorithm(s).

Does it devolve into insertion sort only when your workspace is arranged horizontally? or am I missing something here?

This was causing erroneous block bumps because the connection locations
were changed before the blocks were actually rerendered.
@BeksOmega BeksOmega merged commit f2b75fb into google:develop Mar 1, 2023
Comment on lines +1584 to +1586
queueRender() {
queueRender(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This method doesn't have very many callers, and they are all in either block_dragger.ts or rendered_connection.ts. Do you think indirecting via BlockSvg.prototype.queueRender (rather than calling render_management.ts's queueRender directly) is justified on redability or other grounds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Later changes make several more calls to this, so I think it is justified on grounds of readability. Plus, it's internal, so the maintainance cost to including it is minimal :P If we want to tear it out later we can!

core/insertion_marker_manager.ts Show resolved Hide resolved
core/render_management.ts Show resolved Hide resolved
core/rendered_connection.ts Show resolved Hide resolved
@BeksOmega BeksOmega mentioned this pull request Mar 6, 2023
4 tasks
@BeksOmega BeksOmega deleted the feat/render-queue branch September 21, 2023 13:21
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance of inserting blocks into stacks.
5 participants