Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

BufferedProcess: search only new data for new lines rather than entire buffer #10930

Merged
merged 1 commit into from
Mar 14, 2016

Conversation

chuckhendo
Copy link
Contributor

I've noticed for some time that viewing installed packages inside of settings-view was very slow. I did a little research on it, and reported the issue on the settings-view repo here atom/settings-view#734. It seems the issue is due to BufferedProcess trying to find new lines on the entire stream buffer, including data that it's already searched for new lines on.

This PR fixes that by only searching the new data from the stream. Viewing installed packages is now significantly faster. I've attached a before and after screenshot of the CPU profile when opening settings-view and then going to the Packages tab.

Before

screen shot 2016-02-22 at 3 24 51 pm

After:

screen shot 2016-02-22 at 3 25 30 pm

@50Wliu
Copy link
Contributor

50Wliu commented Feb 22, 2016

Very nice! Can you please add some specs to this?

@Zireael07
Copy link

3300 ms -> 8.5 ms. Is it a speedup record? 🏁 💥

@mehcode
Copy link
Contributor

mehcode commented Feb 23, 2016

Very nice! Can you please add some specs to this?

I'm curious what specs would be needed. This is a few line change that significantly improves performance. If the existing specs pass, isn't that enough? It's not like new (and testable) functionality is being introduced here.

If we have no specs for that function and you're asking for @chuckhendo to write the specs from ground zero ... that's not something I'd do as a project maintainer but maybe Atom works differently.

@chuckhendo
Copy link
Contributor Author

I certainly don't mind writing specs, but agreed with @mehcode - I'm not exactly sure what I should write. Maybe something to test stdout to make sure it's properly separating lines?

@lee-dohm
Copy link
Contributor

I don't think that asking for specs is unreasonable, especially given that BufferedProcess is a part of the public Atom API. Even just a test that it doesn't duplicate or drop content would be good 😀 Properly separating lines would be even better!

@joshaber
Copy link
Contributor

Thanks for doing all the heavy lifting on this @chuckhendo!

@chuckhendo
Copy link
Contributor Author

@joshaber Thank you for taking care of the specs! I had that on my todo list, but then I left for vacation. Glad to see this get merged in

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants