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

Proof-of-concept: lazifying measureLineInner #1613

Closed
wants to merge 5 commits into from

Conversation

aslushnikov
Copy link
Contributor

This is a proof-of-concept that does lazy measurement of long lines and partly addresses issue #1022.
To be more specific, when one requests measurement information regarding char N, it
measures information for characters in [N-50, N+50]. The rendering is done in a following fashion:

  • tokens that are before [N-50, N+50] range are rendered as-is
  • tokens that fall inside [N-50, N+50] range are splitted into chars

With this patch rendering a single line with 75k characters gets done in 300ms instead of 16000ms.

It's not a finished thing and it has couple of unhandled cases; though this effort should obsolete pull request #1609 as it works fast, doesn't require monospaced font and supports (in theory) wrapped lines.

How do you feel about it?

@marijnh
Copy link
Member

marijnh commented Jun 14, 2013

I agree that this might have more future than the other approach. Have you thought about how to address the fact that coordsChar does a binary search through the whole line, and thus jumps all over, fetching multiple ranges on long lines?

@aslushnikov
Copy link
Contributor Author

I have some thoughts about this that should considerably reduce amount of large hops; i'll try to implement this after I polish/finish this patch.
Btw, will you consider landing this without coordsChar improvement?

@marijnh
Copy link
Member

marijnh commented Jun 17, 2013

I haven't read this in detail yet, but yes, in principle I am open to merging something like it -- coordsChar probably won't get much worse (maybe we'll have to increase the size of the cache a bit, since the maximum size of cached measurements is a lot smaller with this approach).

I'll spend some more time on this next week, after 3.14 is released -- I try not to merge scary patches shortly before releases.

@aslushnikov
Copy link
Contributor Author

maybe we'll have to increase the size of the cache a bit, since the maximum size of cached measurements is a lot smaller with this approach

Looks like there's no need for this: measurements are not recreated but full-filled with missing info instead.

@aslushnikov
Copy link
Contributor Author

Status update:

  • the patch poorly handles spans created by CodeMirror.markText method (these spans should be handled with care). As a result, mark-selection addon becomes unusable.
  • the patch has some problems with selection of arabic text (don't have any idea why)
  • there are issues with wrapped lines support, specifically cursor height sometimes is not big enough (might be fixed via changing measurement area from 50 to, say, 300)
  • setting cursor via mouse doesn't work instantly as it does series of measurements with forced layouts. Good news this could be sped up.

To sum up, at the moment this patch works reasonably good for non-wrapped code editors that does not heavily use 'markText' functionality, which sounds really promising. Still, there is a lot to be done to bring it to finished state.

@aslushnikov
Copy link
Contributor Author

Do you still plan on working on this?

@marijnh
Copy link
Member

marijnh commented Jul 2, 2013

It does not look like this is solid enough to merge, or easy to adjust. If someone (Google? @ibdknox?) wants to pay for two weeks of my time to do the serious engineering needed to land a feature like that, that'd work for me. But as it stands, this looks too destabilizing for me to risk landing it.

@ibdknox
Copy link
Contributor

ibdknox commented Jul 3, 2013

That may be a possibility, I've been thinking about working with you on a general "attack performance" gig (specifically this and line numbers). Not quite at that point though.

@marijnh
Copy link
Member

marijnh commented Jan 24, 2014

Something like this now exists in the v4 branch. Closing this.

@marijnh marijnh closed this Jan 24, 2014
@aslushnikov aslushnikov deleted the lazy-measure branch October 20, 2014 14:44
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.

3 participants