fmoo / react-typeahead

Pure react-based typeahead and typeahead-tokenizer
ISC License
677 stars 231 forks source link

Short circuit updating visible when entry is empty #125

Closed trshafer closed 9 years ago

trshafer commented 9 years ago

@thehuey,

Here's a PR that includes a benchmark test. It's run by calling npm run benchmark.

This is the output I get:

running with 10000 items
original x 38.72 ops/sec ±1.35% (52 runs sampled)
short circuit x 6,076 ops/sec ±1.91% (85 runs sampled)
Fastest is short circuit

The effect is dramatic. And it gets much more dramatic the more items in the list.

I don't know if you want to add the option searchOnBlankValue or not, but I needed it to run the benchmark.

Addresses #124.

thehuey commented 9 years ago

I just took a look at the _generateFilterFunction. It seems the inefficiency is coming from the transformedOptions mapping call in addition to the fuzzy.filter. @seansfkelley, can we rewrite this to use the fuzzy.filter(value, list, options) call? passing options = { extract: filterOptionProp } should let fuzzy.filter run through the list once without having to clone the values.

trshafer commented 9 years ago

I do think that will help. However it will still be iterating over the entire list. I'll give it a shot.

trshafer commented 9 years ago

I just changed it.

Still get these results:

running with 10000 items
original x 40.22 ops/sec ±1.39% (54 runs sampled)
short circuit x 6,173 ops/sec ±1.57% (87 runs sampled)
Fastest is short circuit

No statistical difference with the change.

trshafer commented 9 years ago

It's a good change because it's less code and unnecessary work is bad. However the issue is the fuzzy search is slow. It's ok that it's slow. This issue is really about preventing a search when nothing has changed.

thehuey commented 9 years ago

I was thinking you can do both. Fix the transformedOptions, and put the short circuit in.

However it does look like the map function doesn't improve it much at all.

Last suggestion, put your circuit breaker in the getOptionsForValue function. Then you'll only need to specify it once.

thehuey commented 9 years ago

If you could squash your commits, that would help too.

trshafer commented 9 years ago

@thehuey, do you want the prop searchOnBlankValue? If you don't there's no need for the benchmark.

trshafer commented 9 years ago

I fixed up the commits and removed the benchmark.

trshafer commented 9 years ago

Original pr source is here: https://github.com/amplii/react-typeahead/tree/short-circuit-updating-visible-when-entry-is-empty-benchmark

thehuey commented 9 years ago

Looks good to me. :+1: Will see if @fmoo can merge this in.

fmoo commented 9 years ago

1.1.4 published to npm. This is awesome. Thanks for your help @thehuey, @growlsworth !