Skip to content

some operators use useMap and prefix #137

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

Merged
merged 1 commit into from
Jan 25, 2021
Merged

some operators use useMap and prefix #137

merged 1 commit into from
Jan 25, 2021

Conversation

staltz
Copy link
Member

@staltz staltz commented Jan 25, 2021

Before and after of the perf benchmarks:

  migration (using ssb-db)

    ✔ delete db2 folder to start clean
    ✔ ssb-db has finished indexing
-   ✔ duration: 2816ms
+   ✔ duration: 2870ms
    fallback to close

  migration (alone)

    ✔ delete db2 folder to start clean
-   ✔ duration: 2705ms
+   ✔ duration: 2722ms

  initial indexing

    ✔ null
-   ✔ duration: 2008ms
+   ✔ duration: 2043ms

  setup

  key one initial
-   ✔ duration: 454ms
+   ✔ duration: 464ms

  key two
-   ✔ duration: 19ms
+   ✔ duration: 20ms

  key one again
-   ✔ duration: 14ms
+   ✔ duration: 15ms

  latest root posts
-   ✔ duration: 371ms
+   ✔ duration: 364ms

  latest posts
-   ✔ duration: 29ms
+   ✔ duration: 32ms

  votes one initial
-   ✔ duration: 355ms
+   ✔ duration: 383ms

  votes again
-   ✔ duration: 11ms
+   ✔ duration: 0ms

  hasRoot
-   ✔ duration: 302ms
+   ✔ duration: 294ms

  hasRoot again
-   ✔ duration: 12ms
+   ✔ duration: 0ms

  author one posts
-   ✔ duration: 248ms
+   ✔ duration: 249ms

  author two posts
-   ✔ duration: 61ms
+   ✔ duration: 66ms

  teardown

  total:     18
  passing:   18
  duration:  15.3s

@staltz staltz requested a review from arj03 January 25, 2021 11:09
@arj03
Copy link
Member

arj03 commented Jan 25, 2021

Sweet. I had a similar PR here locally. Wanted to get the concurrent update in, but this is good. We should use prefix for key as well. It's about 2x faster. But there is still quite a bit of key collision (around 1/5) so I'll try and maybe do the % trick and see if that helps. Anyway this needs a benchmark update as well, so lets just get this in first and I'll look at key afterwards.

@arj03 arj03 merged commit bf99302 into master Jan 25, 2021
@arj03 arj03 deleted the use-map-prefix branch January 25, 2021 11:15
@github-actions
Copy link

Benchmark results

Part Duration
Migration (using ssb-db) 8271ms
Migration (alone) 3515ms
Initial indexing 2471ms
key one initial 764ms
key two 3ms
key one again 3ms
latest root posts 521ms
latest posts 10ms
votes one initial 533ms
votes again 0ms
hasRoot 396ms
hasRoot again 1ms
author one posts 317ms
author two posts 59ms

@staltz
Copy link
Member Author

staltz commented Jan 25, 2021

Sweet. I had a similar PR here locally. Wanted to get the concurrent update in, but this is good.

Oh, I didn't know you had a branch. Sorry about that.

We should use prefix for key as well. It's about 2x faster.

key() operator currently uses prefix indexes (but not useMap), so what makes it 2x faster?

@github-actions
Copy link

Benchmark results

Part Duration
Migration (using ssb-db) 8984ms
Migration (alone) 3789ms
Initial indexing 2457ms
key one initial 816ms
key two 6ms
key one again 4ms
latest root posts 480ms
latest posts 16ms
votes one initial 491ms
votes again 1ms
hasRoot 320ms
hasRoot again 0ms
author one posts 303ms
author two posts 65ms

@arj03
Copy link
Member

arj03 commented Jan 25, 2021

Don't worry about the branch, it was mostly for testing this stuff.

Key is 2x faster with useMap

@staltz
Copy link
Member Author

staltz commented Jan 25, 2021

Interesting! I thought key() would be like author(), and that author() doesn't benefit from useMap, see your comment here: ssbc/jitdb#95 (comment)

@arj03
Copy link
Member

arj03 commented Jan 25, 2021

Yeah, changing this line from 0 to 1 makes key with useMap go from 10ms to < 1ms.

@arj03
Copy link
Member

arj03 commented Jan 25, 2021

But maybe we should add that as an option? prefixOffset?

@staltz
Copy link
Member Author

staltz commented Jan 25, 2021

But maybe we should add that as an option? prefixOffset?

I was typing this right now. :)
I was going to suggest prefixStart but prefixOffset is better.

@arj03
Copy link
Member

arj03 commented Jan 25, 2021

Cool, will work on that after lunch.

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