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

Optimize clearing of markers during buffer search #835

Merged
merged 5 commits into from Dec 29, 2016

Conversation

Projects
None yet
3 participants
@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Dec 29, 2016

Depends on atom/text-buffer#192

@maxbrunsfeld maxbrunsfeld merged commit 4bd4df9 into master Dec 29, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-optimize-buffer-search branch Dec 29, 2016

@jedwards1211

This comment has been minimized.

Copy link

jedwards1211 commented on lib/buffer-search.js in 1b87500 Aug 11, 2018

even after years of doing JS, I still don't feel any easier about the performance of indexOf or splice usage like this

This comment has been minimized.

Copy link

jedwards1211 replied Aug 11, 2018

this does mean atom would be doing a half trillion array assignments should I ever need to replace a million occurrences of some text, right?

This comment has been minimized.

Copy link
Contributor Author

maxbrunsfeld replied Aug 11, 2018

V8 probably implements this splice as a call to Memcpy so that it’s vectorized and pretty fast. If you’re curious, try profiling this code path with a large search.

This comment has been minimized.

Copy link

jedwards1211 replied Aug 11, 2018

That's true... in case, we can use Sets in Atom now, right? Would you accept a PR to change this to a Set?

This comment has been minimized.

Copy link

jedwards1211 replied Sep 10, 2018

@maxbrunsfeld related: #653

This comment was marked as disruptive content.

Copy link

jedwards1211 replied Sep 10, 2018

also, just because V8 might be able to do this fairly quickly, doesn't mean this code isn't sinfully wasteful of computation.

This comment has been minimized.

Copy link

jedwards1211 replied Sep 10, 2018

Given that you contributed to the admirable efforts to develop native high performance text buffers for Atom, I'm surprised you're willing to accept this code?

This comment has been minimized.

Copy link
Member

lee-dohm replied Sep 10, 2018

@jedwards1211 one of your comments was minimized as a violation of the Atom Code of Conduct as it was considered to be "shaming" and thus insulting or derogatory. You may consider this an official warning.

This comment has been minimized.

Copy link
Contributor Author

maxbrunsfeld replied Sep 10, 2018

@jedwards1211 I'm not sure why you're commenting about this here. This commit just converts this code from CoffeeScript to JavaScript.

I'll ask again though: could you try profiling this code before you conclude that this function has a significant performance impact? That is how you do performance work.

This comment has been minimized.

Copy link

jedwards1211 replied Sep 11, 2018

Yeah I could have taken the time to dig back through the history of the original coffeescript file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment