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

Replace zee-highlight with tree sitter queries #29

Merged
merged 2 commits into from
Jul 17, 2022

Conversation

mcobzarenco
Copy link
Collaborator

No description provided.

@mcobzarenco mcobzarenco self-assigned this May 1, 2022
@mcobzarenco mcobzarenco force-pushed the highlight-with-queries branch 2 times, most recently from 2fc3d76 to 8a86bbc Compare May 2, 2022 20:30
@iainh
Copy link
Contributor

iainh commented Jul 13, 2022

I've been working with this branch a little in my own tree since it is the future and it seems great but I've noticed that with a large number of modes the application start time is poor (~4 seconds on an m1 mac mini with this configuration. This configuration file is a port of the the Helix language configuration file so the argument could be made that users will want something similar as the out of the box experience.

The time can be reduced slightly by using a parallel iterator when loading modes creating the editor context but I wonder if the real solution would be to load modes on demand when opening/creating files. I've been looking into making this but haven't been able to satisfy the borrow checker as of yet.

@mcobzarenco
Copy link
Collaborator Author

mcobzarenco commented Jul 16, 2022

@iainh Thanks for trying out this PR! It has motivated me to come back to it and finish the changes.

I've noticed that with a large number of modes the application start time is poor (~4 seconds on an m1 mac mini with this configuration.

I have been investigating this. FWIW, this is not the case for me, the editor starts instantly for me on ubuntu -- I don't think it comes down to the cpu, it may be because of a difference between how macos vs linux load shared libraries.

The tree sitter parsers are compiled to shared libraries and loaded at runtime using libloading's Library::new()-- digging into the details, I see that on unix, loading a library uses this flag RTLD_LAZY, see os::unix::Library::new(). This should delay loading the library -- I was wondering if somehow in macos this doesn't work? Just a theory though.

I'm investigating what it would mean to delay the call to Library::new() as you suggest -- loading the libraries eagerly has the advantage that you get to see logs / errors for the configuration early on. Maybe we can make the behaviour, lazy vs eager, controllable by a cmd line argument, to use in CI.

@mcobzarenco
Copy link
Collaborator Author

Ha, actually I'm not sure that's correct -- it looks like the code immediately calls a function from the shared library, forcing loading the library

    let language = unsafe {
        let language_fn: Symbol<unsafe extern "C" fn() -> Language> = library
            .get(language_fn_name.as_bytes())
            .with_context(|| format!("Failed to load symbol {language_fn_name}"))?;
        language_fn()
    };

@mcobzarenco
Copy link
Collaborator Author

@iainh I've just pushed a commit that changes to loading shared libraries for tree sitter lazily -- I'd be grateful if you could check it out locally and let me know if you notice better performance on startup. (I also pushed force previously, so you'll have to reset --hard).

Loading libraries lazily is a good idea all around I think, but as mentioned, on Ubuntu 20.04, this make no noticeable difference on startup time. The only downside is that I'd like to have a command line arg to force loading all libraries for CI.

@kevinmatthes
Copy link
Contributor

Whitespace cropping on saving can still break the application. The Colour Panic described #42 is still a symptom therefore.

@iainh
Copy link
Contributor

iainh commented Jul 17, 2022

@mcobzarenco I can confirm that with this change there is no longer a noticeable delay when starting with the configuration file previously linked. Where as previously the application would appear to hang on startup it now comes up instantly.

One interesting test case on my machine is the swift mode. It takes about 1.6 seconds to load and with the old code would block the UI but with the latest change zee comes up instantly, displays the splash screen and then displays the file once the mode finishes loading. I think this is a better user experience than previously but perhaps could be even better with some sort of "loading" message.

@mcobzarenco
Copy link
Collaborator Author

@iainh That's great to hear 🎉

It takes about 1.6 seconds to load and with the old code would block the UI but with the latest change zee comes up instantly, displays the splash screen and then displays the file once the mode finishes loading. I think this is a better user experience than previously but perhaps could be even better with some sort of "loading" message.

👍 some sort of loading screen would be nice

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