Mutate newDirtyPaths instead of creating a new array every iteration

[brendancarney]

Is this adding or improving a feature or fixing a bug?

Performance.

What's the new behavior?

Mutate newDirtyPaths on each iteration instead of copying it. For my unscientific paste test, this brings a 4s paste to about 2.5s.

Have you checked that...?

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn prettier.)
• The relevant examples still work. (Run examples with yarn watch.)

[zhujinxuan]

Hi, can I know how you test it?

I remember both push.apply and concat have similar speed. (https://jsperf.com/array-concat-vs-push-2/17)

Can you test another optimization like this:

    const dirty = Array.prototype.concat.apply(
        newDirtyPath, 
        this.tmp.dirty.map(path => {      
           path = PathUtils.create(path)
          return PathUtils.transform(path, operation).toArray()
     })
   );

I remember concat.apply is faster than reduce(... concat) (but map is slow however....)

[brendancarney]

Good call @zhujinxuan!

I've only been testing in browser so far. I haven't spent time to set up a reliable benchmark in the Slate repo yet though. I did set up a test to simulate the behavior: https://jsperf.com/push-and-concat/3

It looks like the optimization you suggested is a winner. Please check my work if you're able to see if it accurately reflects the code.

Summary:
Original: Slowest
Optimization w/ push in loop: Faster (current PR)

Optimization w/ concat using es6: Same as above

newDirtyPaths.concat(...dirtyPaths)

Optimization w/ Array.prototype.concat (Your suggestion): Fastest!

[brendancarney]

Unfortunately, I think I spoke too soon. The test is slower in my actual slate example, so building the large array is likely slow. I'm concerned that different cases will have different results, so I'm going to keep experimenting and try to find a more reliable optimization here.

[zhujinxuan]

@brendancarney Can I have a loook at the benchmark result by:

1. in master branch, yarn build && yarn benchmark:save
2. in your branch, yarn benchmark

The test time is not a direct measure of benchmark, the test is related to too many things (babel parser for example), I would suggest to use slate's own benchmark.

[brendancarney]

Sorry for the delay, I was out of town for a week. Unfortunately, I'm not sure there's a benchmark accurately reflecting my use case. Running the benchmarks show no meaningful improvements in my branch.

However, I took a recent production example of a large document and tried pasting it into an editor when running the slate examples. Over several attempts with both branches, here are the rough averages:
master: ~60s
this PR: ~18s

Still really slow in both cases unfortunately. I understand that we'll probably want a benchmark that shows this improvement, so I'll try to get that added soon.

[brendancarney]

Ok so I sort of have a benchmark ready, but still trying to figure out the best way to introduce it. Currently, we run each benchmark a minimum of 100 times. This is a little problematic for this specific case because it is least performant with larger documents, and it's really really slow. So running it one hundred times is probably not great. Should we make these options configurable by benchmark?

Here are the results from inserting a paragraph block 1,000 times, wrapped in editor.withoutNormalizing:

★ insert-block:
     user: 0.02 -> 0.07 ops/sec (217.23% faster 🙌 )
     real: 0.02 -> 0.08 ops/sec (230.25% faster 🙌 )

As you can see, still really really slow, but there is an improvement.

[zhujinxuan]

@brendancarney The benchmark is very naive at this moment. But I think the 200% is very convincing.

If you are interested, you can temporarily add a large document insert benchmark inside benchmark/slate/changes and then run it with only yarn benchmark:save --grep pattern and yarn benchmark --grep pattern (where pattern shall be a RegExp).

BTW, can you have a try on the Array.prototype.concat.apply version in the paste large text example? (Perhaps not faster because map is slow...)

I am a bit surprising that the paste took 18 seconds... Last year I did same test after #1661, it was taking 5 seconds (though still slow). I think I may need to investigate why when I have time.

[brendancarney]

Hi @zhujinxuan. It turns out they are about the same. I think your suggestion is slightly more readable. Thoughts?

Also, I did not commit a benchmark because it would be ridiculously slow with the current setup.

To test the speed difference, you can cut and paste the large document example in master, and then in here.

[zhujinxuan]

@brendancarney As there is insignificant difference between concat <<< map and push, I would suggest keep your push version.

Update: I was wrong, the concat version seems more readable.

[ianstormtaylor]

Hey @brendancarney thanks for this! Looks great to me. Just added one thing to fix inline and then I'm good to merge.

[ianstormtaylor]

Is there any reason for collapsing this into a one-liner? Could you re-expand it to be multiple lines like it was previously? I've found that having one-liners like this makes it harder to debug, since setting breakpoints to check values isn't as easy.

[brendancarney]

For sure!

[ianstormtaylor]

Thanks @brendancarney!