search

shurcooL / Go-Package-Store

An app that displays updates for the Go packages in your GOPATH.
MIT License
900 stars 29 forks source link

Update components for render efficiency #86

Closed pdf closed 6 years ago

pdf commented 6 years ago

As discussed in https://github.com/gopherjs/vecty/issues/149#issuecomment-330736079, this is a quick pass at making GPS render more efficiently. As we've just landed keyed children, this seemed like an opportune moment to have a look at this.

Let me start with some background details, and I'll leave some line comments, to further explain what I'm aiming for here.

First off, I think it's valuable to read React's Reconcilliation docs to understand what the renderer has to do on every update, as almost every concept there maps directly to how Vecty operates. The gist is that if children of an element are unstable, and change type, this results in an expensive operation to tear down the entire tree of each such affected child, and replace it with a new one. To expand on that - if an element is removed, every sibling below that element will at minimum need to be mutated, and possibly replaced entirely.

React makes it slightly more difficult for you to shoot yourself in the performance foot, as its render values are (mostly-)logic-free templated blocks of text, so it's more obvious if you're doing something unusual, like rendering a list of elements that may change length between renders, since you need to produce that list outside the normal rendering system. Vecty's programmatic approach is more flexible, but that flexibility can lead to performance loss if abused. Vecty does now have tools to help with these scenarios though: vecty.List; vecty.If; and vecty.Keyer.

Appending to a slice of []vecty.MarkupOrChild, or []vecty.ComponentOrHTML with variable content is a sure-fire way to encounter poor render performance. So, I've made liberal use of vecty.List improve various sections of these components, and implemented vecty.Keyers on the components I think are applicable. I've also added prop tags to component fields where necessary - the justification for these is more involved, but I think it should become more clear once we have a dispatcher implementation (and documentation).

There's obviously no obligation to take these changes as-is, and I've not run them against a production build (since I'm not sure how to do that), but I hope they're at least helpful as an example. These changes are not exhaustive either, there are probably other improvements that could be made.

I'd certainly appreciate any feedback you have.

pdf commented 6 years ago

It's quite late here, so I'll have to leave this as-is for now, please let me know if you would like further explanation.

dmitshur commented 6 years ago

Thanks a lot for sending this PR and a detailed explanation @pdf! I plan to look at it up close tomorrow or later this week.

dmitshur commented 6 years ago

I've not run them against a production build (since I'm not sure how to do that)

For reference, if you're able to do a development build, running a production one should be very easy. It's documented at https://github.com/shurcooL/Go-Package-Store#development, but basically you'd need to run go generate github.com/shurcooL/Go-Package-Store/..., then go install github.com/shurcooL/Go-Package-Store/cmd/Go-Package-Store and run the Go-Package-Store binary.

dmitshur commented 6 years ago

There's obviously no obligation to take these changes as-is, and I've not run them against a production build (since I'm not sure how to do that), but I hope they're at least helpful as an example. These changes are not exhaustive either, there are probably other improvements that could be made.

I'd certainly appreciate any feedback you have.

Thanks a lot again for making this PR @pdf. It took a bit longer for me to get to it, but after spending some time with it now, here are my thoughts.

First, since the purpose of this PR is to improve rendering performance, I've done some rough comparison of the current master rendering performance vs this PR by looking at renderBody timing numbers, and I did not see a performance improvement. Instead, I saw performance to be roughly around 20-100% slower. (However, I only tested a subset of rendering behaviors, perhaps there were improvements to some more rare/harder-to-recreate situations I hadn't tested.)

This PR also fixes incorrect rendering behavior (i.e., https://github.com/gopherjs/vecty/issues/149), so perhaps that's what offsets its performance improvements. Anyway, it's not possible to compare rendering performance until rendering behavior of master is made correct.

Because of that, I would prefer to use this PR as a reference/example. First, I will apply the minimal change to fix incorrect rendering. Then, I'll break this PR up into smaller independent changes, and apply them while testing and gaining understanding for each change individually, and doing benchmarks that give me an idea of what kind of impact each change has on performance.

dmitshur commented 6 years ago

I've left additional thoughts inline. I'm going to close this PR because there's nothing actionable that needs to be done to merge this. As I said, I will use it as an example/reference for future changes.

Also, rendering performance is generally not a bottleneck for GPS needs at this time, so in general, I prefer to put greater focus on readability/organization of code and ability to modify it, rather than raw performance. So I'll be able apply performance changes that don't compromise readability, but the ones that do, I will need to think harder about them to justify them.

Finally, since it's not a priority, I will potentially defer working on this until a later time.

Thank you again!

pdf commented 6 years ago

First, since the purpose of this PR is to improve rendering performance, I've done some rough comparison of the current master rendering performance vs this PR by looking at renderBody timing numbers, and I did not see a performance improvement. Instead, I saw performance to be roughly around 20-100% slower. (However, I only tested a subset of rendering behaviors, perhaps there were improvements to some more rare/harder-to-recreate situations I hadn't tested.)

This surprises me greatly, I will do some profiling. Can I ask exactly how your testing was performed?

pdf commented 6 years ago

I cannot reproduce the performance claims here using a profiler. Paint times appear to be slightly improved with this PR, and everything else appears to be a pretty equal.

dmitshur commented 6 years ago

Thanks for the replies; I'll take them into account.

This surprises me greatly, I will do some profiling. Can I ask exactly how your testing was performed?

I opened the dev console and looked at "renderBody" timings. It measures the time to do vecty.Rerender(body):

https://github.com/shurcooL/Go-Package-Store/blob/eefe1dbecd6146562f95688de1196eab2d031414/frontend/main.go#L138-L143

I tried it for the mock.html page, where the difference was small (10-20%), and on on the live set of updates I had (around 20~ repos), where the difference was larger (80-100%). I just looked at the numbers before and after and estimated the difference, so it's pretty rough.

However, I can't seem to reproduce what I saw earlier now. It's very strange, I'm seeing 0-2 ms times:

image

Maybe I'm grossly misremembering things, but I don't think I've ever seen them so low before! 😮 I'll have to see what happened/changed.

pdf commented 6 years ago

Oh, if you're on the latest vecty master, measuring there will not give you any useful data, as the actual rendering occurs in batches via a requestAnimationFrame callback now. Using a profiler will give you a more accurate picture of what's happening anyway.

dmitshur commented 6 years ago

I'll have to see what happened/changed.

Oh, if you're on the latest vecty master, measuring there will not give you any useful data, as the actual rendering occurs in a requestAnimationFrame callback.

Ah, yeah, that explains it. I updated to latest vecty commit after doing the tests above.