Avoid the possibility of stack overflow in spliceArray helper #204

merged 2 commits into from Feb 10, 2017


None yet

1 participant

maxbrunsfeld commented Feb 10, 2017 edited

We're seeing some errors like this in bugsnag:

RangeError Maximum call stack size exceeded 
    /app.asar/node_modules/text-buffer/lib/helpers.js:18:37 spliceArray
    /app.asar/node_modules/text-buffer/lib/display-layer.js:924:5 updateSpatialIndex
    /app.asar/node_modules/text-buffer/lib/display-layer.js:968:12 populateSpatialIndexIfNeeded
    /app.asar/node_modules/text-buffer/lib/display-layer.js:587:10 getScreenLineCount
    /app.asar/src/text-editor.js:929:32 module.exports.TextEditor.getScreenLineCount
    /packages/minimap/lib/minimap.js:554:28 getHeight

The problem is that we're calling Array.splice with a large number of arguments. We cap the argument count at 100,000, which was an attempt to avoid stack overflows, but depending on how many stack frames deep we are when calling DisplayLayer.updateSpatialIndex, and the size of those stack frames, this cap may still be too high.

The largest splice that I can perform in the dev tools is 125,000. So I think this error may have emerged in 1.14.0 because we are calling splice with 100,000 elements in a slightly deeper stack frame than before. updateSpatialIndex is also a pretty hefty stack frame, because it has a ton of local variables.

I've fixed this problem by rewriting spliceArray to not use Array.splice at all. We now move the items that follow the splice using an explicit loop, and then write the new items into place with another explicit loop. There's less native code being used this way, but we also avoid several large allocations, because we used to repeatedly slice the inserted array, and Array.splice also returns an array of removed items that we weren't using.

/cc @nathansobo

maxbrunsfeld added some commits Feb 10, 2017
@maxbrunsfeld maxbrunsfeld Avoid the possibility of stack overflow in spliceArray helper
@maxbrunsfeld maxbrunsfeld Add mutation benchmark
maxbrunsfeld commented Feb 10, 2017 edited

Just to make sure this doesn't regress performance, I added a really simple benchmark for buffer mutations, and it seems like this might make setTextInRange (plus the DisplayLayer's handling of buffer changes) slightly faster if anything.

@maxbrunsfeld maxbrunsfeld merged commit 4db9d6a into master Feb 10, 2017

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
@maxbrunsfeld maxbrunsfeld deleted the mb-avoid-stack-overflows branch Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment