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

Return out_of_memory error tuple if row alloc fails #304

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Oct 13, 2024

Attempt at #301 (comment)

This PR makes multi_step allocate cells array once, and return an error tuple if that allocation fails.

Some benchmarks (the "10 rows" one might need to be re-run, it seems to be an outlier):

This approach
Mix.install [{:exqlite, github: "ruslandoga/exqlite", branch: "make-row"}, :benchee], force: true

sql = """
with recursive cte(i) as (
  values(0)
  union all
  select i + 1 from cte where i < ?
)
select i, i / 1.0, null, x'000000', 'hello' || i from cte
"""

alias Exqlite.Sqlite3

Benchee.run(
  %{"fetch_all" => fn %{conn: conn, stmt: stmt} -> Sqlite3.fetch_all(conn, stmt) end},
  inputs: %{
    "10 rows" => 10,
    "100 rows" => 100,
    "1000 rows" => 1000,
    "10000 rows" => 10000
  },
  before_scenario: fn rows ->
    {:ok, conn} = Sqlite3.open(":memory:", [:readonly, :nomutex])
    {:ok, stmt} = Sqlite3.prepare(conn, sql)
    Sqlite3.bind(conn, stmt, [rows])
    %{conn: conn, stmt: stmt}
  end,
  after_scenario: fn %{conn: conn, stmt: stmt} ->
    Sqlite3.release(conn, stmt)
    Sqlite3.close(conn)
  end
)

Results:

##### With input 10 rows #####
Name                ips        average  deviation         median         99th %
fetch_all       87.22 K       11.47 μs    ±71.28%        9.83 μs       47.25 μs

##### With input 100 rows #####
Name                ips        average  deviation         median         99th %
fetch_all       11.47 K       87.22 μs    ±14.59%       86.13 μs      128.13 μs

##### With input 1000 rows #####
Name                ips        average  deviation         median         99th %
fetch_all        1.24 K      809.13 μs     ±1.90%      808.29 μs      850.19 μs

##### With input 10000 rows #####
Name                ips        average  deviation         median         99th %
fetch_all        139.13        7.19 ms     ±9.00%        7.45 ms        8.45 ms
Master
Mix.install [:exqlite, :benchee], force: true

sql = """
with recursive cte(i) as (
  values(0)
  union all
  select i + 1 from cte where i < ?
)
select i, i / 1.0, null, x'000000', 'hello' || i from cte
"""

alias Exqlite.Sqlite3

Benchee.run(
  %{"fetch_all" => fn %{conn: conn, stmt: stmt} -> Sqlite3.fetch_all(conn, stmt) end},
  inputs: %{
    "10 rows" => 10,
    "100 rows" => 100,
    "1000 rows" => 1000,
    "10000 rows" => 10000
  },
  before_scenario: fn rows ->
    {:ok, conn} = Sqlite3.open(":memory:", [:readonly, :nomutex])
    {:ok, stmt} = Sqlite3.prepare(conn, sql)
    Sqlite3.bind(conn, stmt, [rows])
    %{conn: conn, stmt: stmt}
  end,
  after_scenario: fn %{conn: conn, stmt: stmt} ->
    Sqlite3.release(conn, stmt)
    Sqlite3.close(conn)
  end
)

Results:

##### With input 10 rows #####
Name                ips        average  deviation         median         99th %
fetch_all       50.65 K       19.74 μs    ±39.01%       18.42 μs       59.86 μs

##### With input 100 rows #####
Name                ips        average  deviation         median         99th %
fetch_all       10.91 K       91.70 μs    ±13.32%       90.00 μs      129.14 μs

##### With input 1000 rows #####
Name                ips        average  deviation         median         99th %
fetch_all        1.19 K      840.99 μs     ±1.63%      840.01 μs      880.36 μs

##### With input 10000 rows #####
Name                ips        average  deviation         median         99th %
fetch_all        130.85        7.64 ms     ±8.54%        7.88 ms        8.82 ms

@ruslandoga ruslandoga changed the title Return errors if row alloc fails Return out_of_memory error tuple if row alloc fails Oct 13, 2024
Copy link
Member

@warmwaffles warmwaffles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have brackets around all if statements. I've been bitten too many times by not having them.

Unsure on what to do when sqlite3_column_count returns 0. The behavior I originally implemented just returned an error, but I don't know if that is appropriate. 🤔

c_src/sqlite3_nif.c Outdated Show resolved Hide resolved
c_src/sqlite3_nif.c Outdated Show resolved Hide resolved
c_src/sqlite3_nif.c Outdated Show resolved Hide resolved
@@ -628,30 +615,47 @@ exqlite_multi_step(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
return make_error_tuple(env, am_invalid_chunk_size);
}

int column_count = sqlite3_column_count(statement->statement);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can return 0

https://www.sqlite.org/c3ref/column_count.html

We should handle this. I don't think an error is appropriate here. I'm not 100% certain.

ruslandoga and others added 3 commits October 14, 2024 20:05
Co-authored-by: Matthew Johnston <warmwaffles@gmail.com>
Co-authored-by: Matthew Johnston <warmwaffles@gmail.com>
Co-authored-by: Matthew Johnston <warmwaffles@gmail.com>
ERL_NIF_TERM* cells = NULL;
cells = enif_alloc(sizeof(ERL_NIF_TERM) * column_count);

if (!cells) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!cells) {
if (!cells && column_count > 0) {

Something like this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I think that would work. It would be safe because the list of results returned would be an empty array.

ERL_NIF_TERM* cells = NULL;
cells = enif_alloc(sizeof(ERL_NIF_TERM) * column_count);

if (!cells) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!cells) {
if (!cells && column_count > 0) {

@warmwaffles
Copy link
Member

This needs to be rebased.

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 this pull request may close these issues.

2 participants