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

Split DatabaseTableUpdate in deletes/inserts vecs #1019

Merged
merged 7 commits into from
Mar 27, 2024
Merged

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 25, 2024

Description of Changes

  • DatabaseTableUpdate is split into two separate vectors for deletes / inserts. This was helpful for performance as it meant we could avoid some temporary allocations.
  • The temporary allocations of Vec<_>s inside eval_{all, lhs, rhs} are removed. Instead, we carry iterators as long as possible. This helped performance.
  • Incr eval now results in two separate vectors for inserts and deletes, so that DatabaseTableUpdateCow mirrors DatabaseTableUpdate. These are only fused at the very moment we convert to bsatn/json. This did not affect performance, but should in theory do less work as well as being cleaner.

Performance

Benchmarking incr-select: Collecting 100 samples in estimated 5.0004 
incr-select             time:   [193.77 ns 196.76 ns 201.67 ns]
                        change: [-55.774% -55.392% -54.906%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  2 (2.00%) high mild
  11 (11.00%) high severe

Benchmarking incr-join: Collecting 100 samples in estimated 5.0049 s 
incr-join               time:   [1.8449 µs 1.8683 µs 1.8983 µs]
                        change: [-30.666% -30.236% -29.737%] (p = 0.00 < 0.05)
                        Performance has improved

Commit 3 should have no noticable perf impact and is mostly cleanup.

Relative to master, on i7-7700K, 64gb ram.

API and ABI breaking changes

None

Expected complexity level and risk

2

Testing

No changes in semantics.

@Centril Centril changed the base branch from master to centril/perf-eval-incr-ref March 25, 2024 12:50
@Centril Centril marked this pull request as ready for review March 25, 2024 15:00
@Centril
Copy link
Contributor Author

Centril commented Mar 25, 2024

@gefjon has already reviewed the contents of this but @joshua-spacetime probably also wants a look.

@bfops bfops added performance test-with-bots Recommend to test under higher CCU release-any To be landed in any release window labels Mar 25, 2024
@bfops
Copy link
Collaborator

bfops commented Mar 25, 2024

Bot test results

Conclusion: ✔️ Possibly some small improvements?

Commits tested

before:

commit cb9d493d46897ee84cc13eeb0fdd7e0063d747af (origin/bfops/perf-test-before)
Merge: af73b29b 0ad01b77
Author: Zeke Foppa <196249+bfops@users.noreply.github.com>
Date:   Mon Mar 25 14:47:25 2024 -0700

    [bfops/perf-test-before]: Merge remote-tracking branch 'origin/bfops/enable-tracy' into bfops/perf-test-before

commit af73b29b5f687c95863772936dafaeea94c5ceaa
Author: Tyler Cloutier <cloutiertyler@aol.com>
Date:   Thu Mar 21 21:19:31 2024 -0700

    Made me the codeowner of the traits.rs file for the datastore

after:

commit 64b692f174247c5a47a4c79472dbdbc9c35c192a (HEAD -> bfops/perf-test-after, origin/bfops/perf-test-after)
Merge: 3fb3e4b8 0ad01b77
Author: Zeke Foppa <196249+bfops@users.noreply.github.com>
Date:   Mon Mar 25 14:47:29 2024 -0700

    [bfops/perf-test-after]: Merge remote-tracking branch 'origin/bfops/enable-tracy' into bfops/perf-test-after

commit 3fb3e4b83703a0af857cf1880a99961fa9670404 (origin/centril/split-dbt)
Author: Mazdak Farrokhzad <twingoow@gmail.com>
Date:   Wed Mar 20 21:37:50 2024 +0100

    store deletes/inserts separately in eval_incr results; mostly cleanup

commit 1ae9f524b5f0fd12207af2e5fbea0d0c8797a4e8
Author: Mazdak Farrokhzad <twingoow@gmail.com>
Date:   Wed Mar 20 14:12:02 2024 +0100

    incr-join: avoid temp Vec<_> allocs

commit d082c00f7588a8bbaddfd8a76b9dee171dcad7cf
Author: Mazdak Farrokhzad <twingoow@gmail.com>
Date:   Wed Mar 20 23:01:06 2024 +0100

    split DatabaseTableUpdate in deletes/inserts vecs

commit 0fc5b2476f866013596b205ea3f18b1b5997397c (origin/centril/perf-eval-incr-ref)
Author: Mazdak Farrokhzad <twingoow@gmail.com>
Date:   Fri Mar 22 00:28:23 2024 +0100

    use MemTable less

call_reducer

image

call_reducer_with_tx

image

eval_incr

image

eval_updates

image

enemy_ai_agent_loop

image

npc_ai_agent_loop

image

@joshua-spacetime
Copy link
Collaborator

DatabaseTableUpdate is split into two separate vectors for deletes / inserts. This was helpful for performance as it meant we could avoid some temporary allocations.

Can you expand on this point? Which allocations does this allow us to get rid of and where?

2. `SourceExpr::{MemTable -> InMemory}`
3. clariy some commentary re. SourceExpr/SourceSet and friends
4. cleanup: simplify `compile_select_eval_incr`
5. remove ProgramStore; twas dead code.
@Centril Centril changed the base branch from centril/perf-eval-incr-ref to master March 27, 2024 20:12
@Centril Centril added this pull request to the merge queue Mar 27, 2024
Merged via the queue into master with commit ddf6048 Mar 27, 2024
6 checks passed
@Centril Centril deleted the centril/split-dbt branch March 28, 2024 20:38
@joshua-spacetime joshua-spacetime linked an issue Mar 28, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window test-with-bots Recommend to test under higher CCU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queries should operate on borrowed product values
3 participants