Skip to content

Conversation

@Centril
Copy link
Contributor

@Centril Centril commented Mar 27, 2024

Description of Changes

This avoids sorting values Vs when inserting into the multi-map.
It should have no impact where there are few values that share the same key, but it might otherwise.

API and ABI breaking changes

None

Expected complexity level and risk

3 - previously this failed in CI, and if it doesn't, we should be sure that it doesn't break some untested semantics before merging.

@Centril Centril changed the title MultiMap: don't sort values; use .push(..) and .swap_remove(..) instead MultiMap: don't sort values; use .push(..) and .swap_remove(..) instead Mar 27, 2024
@bfops
Copy link
Collaborator

bfops commented Mar 27, 2024

Bot test results

Conclusion: Possible improvements in eval*, possible regressions in call*.

Commits tested

before:

commit be567fb52dceee0bce3c5f0d16b3e2b0fee03650 (origin/bfops/perf-test-before)
Merge: b6c0e1c4 0ad01b77
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 13:15:19 2024 -0700

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

commit b6c0e1c4d8accb49b378c9aecba8a3cd5a15ae18 (origin/master, origin/HEAD)
Author: Mazdak Farrokhzad <[email protected]>
Date:   Wed Mar 27 18:05:19 2024 +0100

    Add `AlgebraicValue::take` + move test-code in `btree_index` to tests (#1028)
    
    * add AlgebraicValue::take for a neater interface
    
    * btree_index: move test-only code to tests

after:

commit 8f733ed122965cb21de1ede97a10b97e3175fd40 (HEAD -> bfops/perf-test-after, origin/bfops/perf-test-after)
Merge: 65b2a086 0ad01b77
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 13:15:23 2024 -0700

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

commit 65b2a086566d099db9ccd21ffab9dc41e4634a08
Merge: b6c0e1c4 ea08cdb3
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 13:15:23 2024 -0700

    [bfops/perf-test-after]: Merge commit 'ea08cdb34e1901f31c81aee8a43c22e5c3f35f2d' into bfops/perf-test-after

commit b6c0e1c4d8accb49b378c9aecba8a3cd5a15ae18 (origin/master, origin/HEAD)
Author: Mazdak Farrokhzad <[email protected]>
Date:   Wed Mar 27 18:05:19 2024 +0100

    Add `AlgebraicValue::take` + move test-code in `btree_index` to tests (#1028)
    
    * add AlgebraicValue::take for a neater interface
    
    * btree_index: move test-only code to tests

call_reducer

image

call_reducer_with_tx

image

eval_incr

image

eval_updates

image

eval_binary

image

enemy_ai_agent_loop

image

npc_ai_agent_loop

image

@bfops
Copy link
Collaborator

bfops commented Mar 27, 2024

Bot test results

Conclusion: Possible small savings.

Commits tested

before:

commit 7c37956dd6d6fe81fff6b6c73a2d03518671e5ab (origin/bfops/perf-test-before)
Merge: ddf60485 0ad01b77
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 14:54:44 2024 -0700

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

commit ddf6048547d6f5569a0215aac54d73d431dc552c (origin/master, origin/HEAD)
Author: Mazdak Farrokhzad <[email protected]>
Date:   Wed Mar 27 21:12:30 2024 +0100

     Split `DatabaseTableUpdate` in deletes/inserts vecs  (#1019)
    
    * eval_incr: add RelValue::ProjRef(&PV) to avoid cloning PVs
    
    * 1. rename `build_source_query` -> `in_mem_to_rel_ops`
    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.
    
    * add SourceProvider, simplifying the source set stuff
    
    * use MemTable less
    
    * split DatabaseTableUpdate in deletes/inserts vecs
    
    * incr-join: avoid temp Vec<_> allocs
    
    * store deletes/inserts separately in eval_incr results; mostly cleanup

after:

commit 193e8fcf044d1546324544a6e70bece540f1271f (HEAD -> bfops/perf-test-after, origin/bfops/perf-test-after)
Merge: bf7e32bb 0ad01b77
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 15:05:00 2024 -0700

    [centril/multi-map-unsorted-vals]: Merge remote-tracking branch 'origin/bfops/enable-tracy' into centril/multi-map-unsorted-vals

commit bf7e32bb3c2818dedb70aa7550e1ad49577dd0d9
Merge: ea08cdb3 ddf60485
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 15:04:50 2024 -0700

    [centril/multi-map-unsorted-vals]: Merge remote-tracking branch 'origin/master' into centril/multi-map-unsorted-vals

commit ea08cdb34e1901f31c81aee8a43c22e5c3f35f2d (origin/centril/multi-map-unsorted-vals)
Author: Mazdak Farrokhzad <[email protected]>
Date:   Wed Mar 27 10:52:48 2024 +0100

    multimap: don't sort values, use push & swap_remove

commit 2969cfabf9f9f2082e0761a59796cbe87863d305 (origin/centril/misc-cleanup)
Author: Mazdak Farrokhzad <[email protected]>
Date:   Wed Mar 27 10:16:31 2024 +0100

    btree_index: move test-only code to tests

commit 3a940d4d5ca33a14f216843c58537bede0798e12
Author: Mazdak Farrokhzad <[email protected]>
Date:   Wed Mar 27 10:16:04 2024 +0100

    add AlgebraicValue::take for a neater interface

call_reducer

image

call_reducer_with_tx

image

eval_incr

image

eval_updates

image

eval_binary

image

enemy_ai_agent_loop

image

@Centril Centril force-pushed the centril/multi-map-unsorted-vals branch from ea08cdb to 865e744 Compare April 5, 2024 17:36
@Centril Centril requested a review from gefjon April 5, 2024 18:01
@Centril Centril marked this pull request as ready for review April 5, 2024 18:01
@bfops bfops added the release-any To be landed in any release window label Apr 8, 2024
@Centril Centril added this pull request to the merge queue Apr 22, 2024
Merged via the queue into master with commit 9797695 Apr 22, 2024
@Centril Centril deleted the centril/multi-map-unsorted-vals branch April 22, 2024 10:41
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants