Skip to content
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

Maximum call stack exceeded when initializing Automerge.Text with large (500kb) string #499

Open
cgauld opened this issue Jun 30, 2022 · 4 comments

Comments

@cgauld
Copy link

cgauld commented Jun 30, 2022

Hi!

We're using Preview 6 and have found that initializing Automerge.Text, like this:

doc.text = new Automerge.Text(largeString);

Causes the following error stack when called with a ~500kb string.

Uncaught (in promise) RangeError: Maximum call stack size exceeded
    at updateTextObject (...)
    at interpretPatch (...)
    at getValue (...)
    at applyProperties (...)
    at updateMapObject (...)
    at Context.interpretPatch [as applyPatch] (...)
    at Context.applyAtPath (...)
    at Context.setMapKey (...)
    at Object.set (...)

I haven't spotted an issue for this so I thought I'd add one. Would be grateful to hear of any thoughts/workarounds.

Thanks all!

@ept
Copy link
Member

ept commented Jul 1, 2022

Oh, this is unfortunate. It's probably due to the code using array.splice(index, 0, ...elements) to insert a large number of elements all at once, and JavaScript doesn't like the spread syntax resulting in a method call with hundreds of thousands of arguments.

The quickest workaround for now is probably to insert the text in several chunks, spread across several changes. Something like this:

let doc = Automerge.change(Automerge.init(), doc => doc.text = new Automerge.Text())

for (let chunk of largeString.match(/(.{1,10000})/g)) {
  doc = Automerge.change(doc, doc => doc.text.insertAt(doc.text.length, ...chunk))
}

Not tested, but please give it a try and let me know if it helps!

@michaelpalumbo
Copy link

Interesting. I'm also getting this error, when trying to add a rather large object. I'll see how many properties I'd need to remove before I don't get this error and will report back.

@fo-fo
Copy link

fo-fo commented Aug 17, 2022

I did some testing since I happened to run into this issue with some non-automerge code. In Firefox console the limit seems to be exactly 500k arguments:

(function () {})(...Array(500_000).fill(null))
// result: undefined

(function () {})(...Array(500_001).fill(null))
// result: Uncaught RangeError: too many function arguments

In Chrome console around 125k arguments:

(function () {})(...Array(124_956).fill(null))
// result: undefined

(function () {})(...Array(124_957).fill(null))
// result: Uncaught RangeError: Maximum call stack size exceeded

In Node.js REPL around 120k arguments:

(function () {})(...Array(120_425).fill(null))
// result: undefined

(function () {})(...Array(120_426).fill(null))
// result: RangeError: Maximum call stack size exceeded

Obviously there could be some other things that affect the limit at least in V8 (previous call stack depth?), but there's some ballpark figures.

@ept
Copy link
Member

ept commented Aug 18, 2022

Thanks for those experiments. Looks like the best way of fixing this would be to change the API so that you pass in an array, rather than a variable number of arguments, to insert multiple elements at once. Then we wouldn't be subject to this stack size limitation. However, if we do change the API, it would be best if we could figure out some way of avoiding making it a breaking change. Also, JavaScript's Array.splice() and Array.push() rely on passing multiple arguments; changing them to take a single array argument would make Automerge's API inconsistent with those methods. I'm not sure how best to resolve these conflicting concerns.

echarles pushed a commit to datalayer-externals/automerge-classic-arch that referenced this issue Feb 16, 2023
* Update `Patch` types

* Clarify that the splice patch applies to text

* Add Splice patch type to exports

* Add new patches to javascript
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants