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 HTML's Breakdown performance #79

Closed
wants to merge 1 commit into from

Conversation

BaeBae33
Copy link
Contributor

@BaeBae33 BaeBae33 commented Jul 8, 2022

Signed-off-by: baebae33 baebae233333@gmail.com

What has changed

Hi, Im not good at React. But I've tried to optimize HTML's breakdown performance with kotlin-react-virtual. Please review my code.

Why was it changed

HTML report performs not well with large list.

Related issues

Html file is slow and stopping to respond #74

Signed-off-by: baebae33 <baebae233333@gmail.com>
@simonschiller
Copy link
Collaborator

Thanks for taking the effort to create this PR!

Did you do an before-and-after comparison to see if (and by how much) this improves performance and to make sure virtualization works correctly? I thought that we'd need to introduce a scrolling container for that because fully-height visualization doesn't seem to be supported?

@BaeBae33
Copy link
Contributor Author

BaeBae33 commented Jul 10, 2022

Thanks for taking the effort to create this PR!

Did you do an before-and-after comparison to see if (and by how much) this improves performance and to make sure virtualization works correctly? I thought that we'd need to introduce a scrolling container for that because fully-height visualization doesn't seem to be supported?

Yes. Here's my test result.
Test Code:

@RFunction
fun RBuilder.breakdown(components: List<AppComponent>, sizeType: Measurable.SizeType) {
    val newList = arrayListOf<AppComponent>()
    repeat(300) {
        newList.addAll(components)
    }
    h4(classes = "mb-3") { +"Breakdown (${newList.size} components)" }
    div(classes = "row") {
        componentList(newList, sizeType)
    }
}

Before
The page takes 4s to display.
ruler.before.html
before
Alfter
ruler.alfter.html
The page is displayed immediately.
alfter

But there're some new problems:

  1. It takes much more time (almost 40s) to complete the loading of the whole page.
  2. It takes more time when show/hide files in component than before.

Is this the cost of using react-virtual?
May be my usage is not correct?

@simonschiller
Copy link
Collaborator

I'm no react-virtual expert either (or frontend at all for that matter), but it sounds weird that loading the whole page takes longer and that showing/hiding files is also affected. We should definitely see what is causing this before we merge.

One hunch - we're setting size = components.size, should that actually be the count of elements or should it be the total size in pixels?

@BaeBae33
Copy link
Contributor Author

I'm no react-virtual expert either (or frontend at all for that matter), but it sounds weird that loading the whole page takes longer and that showing/hiding files is also affected. We should definitely see what is causing this before we merge.

One hunch - we're setting size = components.size, should that actually be the count of elements or should it be the total size in pixels?

According to :
https://tanstack.com/virtual/v3/docs/guide/introduction
https://github.com/wellyshen/react-cool-virtual/blob/56d53ff73dd92bf0cf3131a77b855b39437446d4/examples/real-time-resize/Row.js
I tried some example and find it's difficult to use accordion or collapsed in react-virtual. I guess that accordion height must be changed dynamically.

@simonschiller
Copy link
Collaborator

Could be. If that's the case I'd rather not use it though if it doesn't really work with our structure. Maybe we can find another solution to fix the performance problems for large projects, something like a paginated table instead of an accordion maybe 🤔

@BaeBae33 BaeBae33 closed this Jul 28, 2022
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