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

Memory issues in 1.9.0 #561

Closed
pieterbeulque opened this issue Nov 29, 2023 · 39 comments · Fixed by #566
Closed

Memory issues in 1.9.0 #561

pieterbeulque opened this issue Nov 29, 2023 · 39 comments · Fixed by #566

Comments

@pieterbeulque
Copy link

Some of our users are running into memory issues which weren't happening on 1.8.0. I can also confirm that we were able to resolve these issues by downgrading back to 1.8.0.

Some of the console errors when this issue happens:

Database closed

Out of bounds call_indirect

Out of bounds memory access (evaluating 'e.apply(null,s)')

Happens in Safari (Mac), Chrome (Mac & Windows), Edge (Windows), so not an implementation issue, I think.

From some quick Googling it looks like an Emscripten build configuration thing, but I have to admit that I'm absolutely clueless about these things.

If I can do anything more specific to help set up a test case, happy to hear how I can help better.

@lovasoa
Copy link
Member

lovasoa commented Nov 29, 2023

Which version of sql.js are you using? The wasm one? Do you use features like custom js functions?

@lovasoa
Copy link
Member

lovasoa commented Nov 29, 2023

May be related to emscripten-core/emscripten#11208 ?

@kripken, do you have an idea where this could come from ? We bumped emscripten from 3.1.20 to 3.1.49

@pieterbeulque , if you could provide a minimal reproduction, that would be great !

@pieterbeulque
Copy link
Author

wasm, yes! Everything is pretty vanilla, no custom JS functions.

I'll try and free up some time to see if I can whip up a minimal reproduction.

Thank for the quick response.

@kripken
Copy link
Collaborator

kripken commented Nov 29, 2023

Looks like 3.1.20 to 3.1.49 is over a year of updates, so it's hard to guess what could cause this. My best idea would be some change in how LLVM optimizes undefined behavior. I would recommend bisecting on emscripten revisions to find where this regressed, that is usually the fastest method in situations like this:

https://emscripten.org/docs/contributing/developers_guide.html#bisecting

@lovasoa
Copy link
Member

lovasoa commented Nov 29, 2023

Yes, we'll need a good repro from @pieterbeulque for that, thanks @kripken !
Do you think there is undefined behavior in sqlite ? I would find that surprising.

@louisjoecodes
Copy link

+1 sqljs 1.9.0 is making our project run out of memory, pinning the version to 1.8.0 works for us

    const initializer = await sql({
      locateFile: (file: string) =>
        `https://cdnjs.cloudflare.com/ajax/libs/sql.js/1.8.0/${file}`,
    })

@lovasoa
Copy link
Member

lovasoa commented Nov 30, 2023

@louisjoecodes : a small self contained reproduction would be welcome!

@ejechev
Copy link

ejechev commented Dec 6, 2023

+1. Error: out of memory and appears on calling initSqlJs

@lovasoa
Copy link
Member

lovasoa commented Dec 6, 2023

@ejechev : same. Small reproduction welcome.

@lovasoa lovasoa added the invalid label Dec 6, 2023
@pieterbeulque
Copy link
Author

I spent an hour on a minimal repro but we use SQL views very heavily and I'm failing to get it to break in a simpler setup—I'd have to figure out a way to get a snapshot of the database in production that runs out of memory and set up a reproduction with that.

Unfortunately as a solo dev for our company I can't really dedicate more time to this issue, so I'll just be pinning us to 1.8.0 for the time being.

Appreciate the work!

@lovasoa
Copy link
Member

lovasoa commented Dec 6, 2023

Thanks for the efforts anyway, @pieterbeulque ! This is appreciated 👍

@k4ploc
Copy link

k4ploc commented Dec 10, 2023

oh god I was going crazy for not knowing what was wrong, until I compared files with another old application, and the only difference was the wasm file, for the moment I will use version 1.8

thank you for your contributions!

@lovasoa
Copy link
Member

lovasoa commented Dec 10, 2023

@JuanLongines Please provide a self contained reproduction

@k4ploc
Copy link

k4ploc commented Dec 10, 2023

@JuanLongines Please provide a self contained reproduction

Reproduction for me:

create a new angular project

Angular CLI: 17.0.6
Node: 18.17.0      
Package Manager: npm 10.2.3
OS: win32 x64

install dependencies:
npm install --save @capacitor-community/sqlite
apparently this version of angular already has the sql.js version 1.9 package declared in package.json

I created a new service to create the connection to the sqlite database, and run npm serve and I got the error:

Untitled
Untitled

practically my connection was the same because I have it in other apps, I looked for solutions but I couldn't find anything... until I used winmerge to compare files, there were no differences in the files, only the wasm, in winmerge I only looked at the content in hexa and it was different, I decided to copy the wasm from my old project (ver 1.8) and it worked

@iebb
Copy link

iebb commented Dec 13, 2023

similar problem here, popping up "Error: out of memory" for new builds using 1.9.0

@lovasoa
Copy link
Member

lovasoa commented Dec 13, 2023

I think it's clear by now that there is a general problem with the release. What we need is for someone to take the time to investigate it.

@gpetrov
Copy link

gpetrov commented Dec 22, 2023

confirm ver 1.9.0 breaks even the simpliest empty sqlite db creation. See: jepiqueau/jeep-sqlite#33 (comment)
while 1.8.0 works perfect.

@larshp
Copy link

larshp commented Jan 10, 2024

also running into issues with 1.9.0, works on 1.8.0

happens when INSERTing data, running on NodeJs

RuntimeError: memory access out of bounds
    at wasm://wasm/002b23f6:wasm-function[2098]:0x9584b
    at wasm://wasm/002b23f6:wasm-function[1591]:0x7a0fc
    at wasm://wasm/002b23f6:wasm-function[67]:0x269a
    at wasm://wasm/002b23f6:wasm-function[476]:0x2595b
    at wasm://wasm/002b23f6:wasm-function[358]:0x19489
    at wasm://wasm/002b23f6:wasm-function[984]:0x42ddd
    at wasm://wasm/002b23f6:wasm-function[398]:0x1f3ce
    at wasm://wasm/002b23f6:wasm-function[393]:0x1b14e
    at wasm://wasm/002b23f6:wasm-function[386]:0x1a89a
    at wasm://wasm/002b23f6:wasm-function[136]:0x711c

@lovasoa
Copy link
Member

lovasoa commented Jan 10, 2024

@larshp , are you interested in helping debugging this ?

@kripken suggested bisecting on the emscripten version. It's a little bit of work, but it will help a lot of people.

@pieterbeulque
Copy link
Author

I wonder if the new release fixes this regression. If any one of the commenters has a more straight-forward way of testing this than me, I'd love to see you try it out.

@larshp
Copy link

larshp commented Jan 17, 2024

thanks, looks like I'm getting the same error on 1.10.1

RuntimeError: memory access out of bounds
    at wasm://wasm/002badfe:wasm-function[2116]:0x97859
    at wasm://wasm/002badfe:wasm-function[1608]:0x7be53
    at wasm://wasm/002badfe:wasm-function[67]:0x26ae
    at wasm://wasm/002badfe:wasm-function[476]:0x25a87
    at wasm://wasm/002badfe:wasm-function[358]:0x1952b
    at wasm://wasm/002badfe:wasm-function[984]:0x42f60
    at wasm://wasm/002badfe:wasm-function[398]:0x1f510
    at wasm://wasm/002badfe:wasm-function[393]:0x1b293
    at wasm://wasm/002badfe:wasm-function[386]:0x1a9df
    at wasm://wasm/002badfe:wasm-function[136]:0x7130

@pieterbeulque
Copy link
Author

Thanks for verifying @larshp

@lovasoa
Copy link
Member

lovasoa commented Jan 17, 2024

minimal reproduction still welcome

@larshp
Copy link

larshp commented Jan 20, 2024

here we go, https://github.com/larshp/sqljs-crash

the crash https://github.com/larshp/sqljs-crash/actions/runs/7593311451/job/20683652663 with logic https://github.com/larshp/sqljs-crash/blob/main/src/index.js

and the same code working on 1.8.0 larshp/sqljs-crash#1

in my scenario it looks like its happening when inserting data that is longer than 64k bytes?

I'm doing require('sql.js/dist/sql-wasm-debug');, and it seems to work with require('sql.js');, but still it should not crash?

@kripken
Copy link
Collaborator

kripken commented Jan 20, 2024

That 64K number makes me think this might be related to the stack. Does sql.js pass data of arbitrary size there? If so then it could be the default stack size changes in 3.1.27. If that's it then rebuilding with -sSTACK_SIZE=5MB would fix things, as mentioned in the link.

Another way to confirm that is to build with -sSTACK_OVERFLOW_CHECK=2 or -sASSERTIONS=2 which will add (expensive) checks for stack overflows.

If stack size is relevant and sql.js uses pthreads then 3.1.30 also has a relevant change.

@lovasoa
Copy link
Member

lovasoa commented Jan 20, 2024

@kripken : yes, sql.js does put the sql string to execute on the stack. I'll open a pr with the changes you suggest.

lovasoa added a commit that referenced this issue Jan 20, 2024
lovasoa added a commit that referenced this issue Jan 20, 2024
@lovasoa lovasoa removed the invalid label Jan 20, 2024
lovasoa added a commit that referenced this issue Jan 20, 2024
* add a test for #561

* fix memory issue with long sql strings

fixes #561

thanks @kripken for the fix: #561 (comment)

* include the new stack size on all targets

* remove debig log
lovasoa added a commit that referenced this issue Jan 20, 2024
@lovasoa
Copy link
Member

lovasoa commented Jan 20, 2024

I pushed the fix and added a test.

@kripken : if the size of the stack is known at compile time, maybe emscripten could add an inexpensive check in stackAlloc that the requested size is smaller than the known stack size ?

@lovasoa
Copy link
Member

lovasoa commented Jan 20, 2024

@pieterbeulque @gpetrov @iebb : independently of this issue, you should probably change your code to use prepared statements instead of gigantic sql strings. Your code will be simpler, more secure, and faster.

@pieterbeulque
Copy link
Author

Amazing, thank you! I'll upgrade and test it out.

The reason we ran out of memory then is that I sometimes bundle a bunch of SQL queries and join them with a ; and then run them in a single db.run(longStringOfMergedSqlStatements).

This issue wouldn't've happened if I ran db.run() for each of those queries separately?

Thanks again!

@lovasoa
Copy link
Member

lovasoa commented Jan 22, 2024

Yes, the problem was with sql strings longer than ~64Kb.

@kripken
Copy link
Collaborator

kripken commented Jan 22, 2024

if the size of the stack is known at compile time, maybe emscripten could add an inexpensive check in stackAlloc that the requested size is smaller than the known stack size ?

We do have such checks in ASSERTIONS=1. They result in an error like this:

Stack overflow detected.  You can try increasing -sSTACK_SIZE (currently set to 65536)

Looks like we didn't think to try a debug build, but next time we find such a weird issue that's a good thing to do.

@lovasoa
Copy link
Member

lovasoa commented Jan 22, 2024

@kripken : What I meant is: isn't the cost of an if (size > CONSTANT) throw new Error(...) at the beginning of the stackAlloc js function low enough that it could be included by default in production builds in emscripten ?

@kripken
Copy link
Collaborator

kripken commented Jan 22, 2024

I see, you meant in production. In general we don't add assertions to production builds both for code size and for speed. I agree that both would only be very minor regressions in this case, but many different assertions make equal sense, and together they would add up. So our focus is to only add them to debug builds by default. Note that you can make a release build with assertions (e.g. -O3 -sASSERTIONS) which is useful for testing sometimes too.

@k4ploc
Copy link

k4ploc commented Jan 23, 2024

I try with version 1.10.2 and Im getting same error

image

This is my sql query:
add: my table is empty

image

@lovasoa
Copy link
Member

lovasoa commented Jan 23, 2024

Ok, I'll reopen this if you can provide a small self-contained reproduction!

@folsze
Copy link

folsze commented Jan 25, 2024

I am getting this too. I could share a repo that uses this in a library (which depends on sql.js). It's a small react project but definitely not a minimimum reproducible example. Is that OK?

@lovasoa
Copy link
Member

lovasoa commented Jan 25, 2024

Ideally the reproduction should depend only on sql.js. See the reproduction that allowed fixing the previous bug above.

@k4ploc
Copy link

k4ploc commented Jan 25, 2024

@lovasoa Thanks! apparently it was a problem with the dependency in Ionic, they have fixed it too, thanks for the effort!

@lastmjs
Copy link

lastmjs commented Jan 27, 2024

My issue might be related, at least to a few of these comments, though reducing the versions does not help me to avoid the out of memory error: #568

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

Successfully merging a pull request may close this issue.