Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Improved performance by making the CodeMirror element directly inherit ... #1007

Closed
wants to merge 1 commit into from

Conversation

achicu
Copy link
Member

@achicu achicu commented Jun 7, 2012

... from a root element.

This way it avoids the layout of the flex box, which affects both layout runtime and dirty rectangles sizes.

…t from a root element, thus avoiding the layout of the flex box, which affects both layout runtime and dirty rectangles sizes.
@julianasuh
Copy link
Contributor

Peter will evaluate the performance part.

@peterflynn
Copy link
Member

@chicu123: sorry we haven't gotten back to you on this yet. We'd like to spend some cycles testing this (and adobe/CodeMirror2#60) and evaluating its impact. I've added a user story (https://trello.com/card/evaluate-scrolling-performance-enhancements/4f90a6d98f77505d7940ce88/555) to track this work; we're going to try to get it into our next sprint. Thanks again for all the awesome evaluation and suggestions!

@ghost ghost assigned peterflynn Aug 1, 2012
@peterflynn
Copy link
Member

Looks very promising! We did some performance testing on this patch last sprint -- here's a summary of the results.

This patch produced fantastic results! There are some downsides to moving the CodeMirror instance out of the main parent hierarchy, though. I'm hoping that we could get most (if not all) of these performance gains by leaving the editor where it currently is, but changing its parents to avoid using flex-box (which as @chicu123 has pointed out, isn't well optimized in the webkit code).

So, I think what we'd like to do next is develop a variant of this patch per the above idea, and then run another round of tests to see if it still produces a big win. (And then merge it!). @chicu123, that new patch could come either from us or you, depending on who has cycles... But we'll also need to allocate some time on our end for the testing. The next clear opportunity to do that is this story, but sadly that's not scheduled for a while. Hopefully we'll be able to make progress sooner than that.

I'll close this pull request for now since I don't think we'll want to take this exact patch. But again, thanks a ton for this great work! It's really exciting to see how big an impact this change could have.

@peterflynn peterflynn closed this Aug 2, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants