Skip to content
This repository was archived by the owner on May 25, 2021. It is now read-only.

Improve mem3_sync event listener performance#19

Merged
asfgit merged 4 commits into
apache:masterfrom
b20n:2984-mem3-sync-event-listener-perf
May 9, 2016
Merged

Improve mem3_sync event listener performance#19
asfgit merged 4 commits into
apache:masterfrom
b20n:2984-mem3-sync-event-listener-perf

Conversation

@b20n
Copy link
Copy Markdown
Contributor

@b20n b20n commented Apr 10, 2016

There are three fundamental changes here, reflected in the latter three commits. These changes were motivated by observing and profiling mem3_sync's event listener pid during high (10k per second) write throughput load tests on databases with high (300+) q values.

We could conceivably pick and choose any of the three approaches here. The read_concurrency flag seems like a no-brainer and the ets:select/2 switch looks like a win in my limited testing.

I'm ambivalent about the frequency/delay work; it turned out surprisingly subtle (to avoid dropping the "last" update on an otherwise-idle host) and required more LOC than I'd preferred, since the simplest solution wasn't compatible with the way the event listener was being spawned previously.

COUCHDB-2984

@b20n b20n force-pushed the 2984-mem3-sync-event-listener-perf branch 2 times, most recently from 14630f6 to 8268c9a Compare April 10, 2016 20:57
Comment thread src/mem3_shards.erl
named_table,
{keypos,#shard.dbname},
{read_concurrency, true}
]),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed, this makes sense.

@kocolosk
Copy link
Copy Markdown
Member

I think the frequency/delay work has merit; executing superfluous mem3_sync:push operations has a measurable cost.

@mikewallace1979
Copy link
Copy Markdown
Contributor

This looks like a good set of changes to me though I'm dubious whether I am
qualified to give a +1 here.

My only comments concern 5bf9317:

  • It might be helpful to have some module-level comments that give a few
    pointers to the reader on the underlying implementation.
  • The mechanism for ensuring all bucketed shards are pushed in the event of
    no further updates might not be obvious to all future maintainers. It might
    be worth explicitly noting that the gen_server timeout is being set by the
    return value of the callbacks (via maybe_push_shards/1) and that the timeout
    will trigger the handle_info(timeout, St) clause.
  • I can't see how the delay is actually used other than when rebucketing
    the shards in response to a config change.

It also feels like these new config parameters should be documented somewhere
though I'm not exactly sure where that should be.

@b20n
Copy link
Copy Markdown
Contributor Author

b20n commented Apr 17, 2016

@mikewallace1979: I've added a couple of comment blocks to address your first two points. Regarding the third, see here. Configuring "bucket count" would be a more direct representation of the implementation, but I believe "delay" should be simpler for operators because it's directing behavior regardless of implementation details.

@mikewallace1979
Copy link
Copy Markdown
Contributor

@banjiewen Ah that makes sense and I agree that "delay" fits better with operator's understanding of the system than "bucket count".

+1 from me.

@b20n
Copy link
Copy Markdown
Contributor Author

b20n commented Apr 27, 2016

@kocolosk: Mind giving a vote on this one?

@kocolosk
Copy link
Copy Markdown
Member

kocolosk commented May 3, 2016

+1 on the code review. I've not tested the changes myself. I understand that at least some of them have been deployed to test clusters and have yielded substantial improvements. Have all the commits been through that testing? If so, let's merge this one.

@b20n
Copy link
Copy Markdown
Contributor Author

b20n commented May 9, 2016

@kocolosk: This exact changeset hasn't been tested, but a functionally equivalent one has; the Cloudant mem3 isn't quite in sync with the Apache mem3. Performance results were indeed quite positive.

/cc @chewbranca

b20n added 3 commits May 9, 2016 14:20
In high-throughput scenarios on databases with large q values the
mem3_sync event listener becomes overloaded with messages due to the
poor performance of the shard selection logic.

It's not strictly necessary to sync on every update, but we do need to
be careful not to lose updates by keeping history too naively. This
patch adds a configurable delay and push frequencyto reduce pressure on
the mem3_sync event listener.

COUCHDB-2984
The result of mem3_shards:for_db/1 on databases with high q values can
be very large, resulting in suboptimal performance for high-volume
callers.

mem3_sync_event_listener is only interested in a small subset of the
result of mem3_shards:for_db/1; moving this filter in to an ets:select/2
call improves performance significantly.

COUCHDB-2984
This table sees a great deal of activity from various subsystems -
turning on read_concurrency should be a win.

COUCHDB-2984
@b20n b20n force-pushed the 2984-mem3-sync-event-listener-perf branch from 8478c60 to 0b70afb Compare May 9, 2016 21:21
@b20n
Copy link
Copy Markdown
Contributor Author

b20n commented May 9, 2016

Squashed.

@asfgit asfgit merged commit 0b70afb into apache:master May 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants